[treeplayer] Don't mistake formulas for script files in TTree::Draw by guitargeek · Pull Request #22774 · root-project/root

guitargeek · 2026-07-08T08:39:13Z

TTree::Draw can take the name of a C++ script file instead of a TTreeFormula expression for both the variable expression and the selection. To decide between the two, TTreePlayer::DrawSelect checked whether a matching file exists via TSystem::IsFileInIncludePath(), which strips a trailing parenthesized part as ACLiC-style macro arguments. A formula like "abs(Bs_TRUEID)" was thus reduced to "abs", and if an unrelated file with that name happened to exist in the current directory, the Draw call failed:

root [0] tree->Draw("Bs_MCORR", "abs(Bs_TRUEID)")
Error in <TTreePlayer::DrawSelect>: Drawing using a C++ macro
currently requires that both the expression and the selection are
files: "Bs_MCORR" is not a file

Two inconsistencies made the trap wider: the selection branch did not require a dot in the candidate string at all, and the varexp branch checked for a dot before stripping the arguments, so a decimal constant inside the parentheses (e.g. "abs(x+0.5)") satisfied it.

Factor the decision into a single helper used for both arguments, which requires the candidate - after stripping the ACLiC mode and arguments - to have an extension. This is not a new restriction: TTreeProxyGenerator::WriteProxy() already refuses a script without an extension (the function name is derived from the basename minus the extension, for the cut script too), so such a file could never be processed anyway. It now evaluates as a formula instead of failing with a misleading error.

Add regression tests that exercise the previously failing Draw calls in the presence of a shadowing file named "abs", and that check a real "abs.C" macro is still recognized as a script because of its extension.

Fixes https://its.cern.ch/jira/browse/ROOT-8000

🤖 Done with the help of AI

github-actions · 2026-07-08T10:55:26Z

Test Results

23 files 23 suites 3d 17h 14m 59s ⏱️
3 877 tests 3 876 ✅ 0 💤 1 ❌
78 960 runs 78 959 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit f7edbf1.

♻️ This comment has been updated with latest results.

dpiparo · 2026-07-11T05:46:22Z

+/// in the TTree::Draw documentation. Determine whether 'expression' names
+/// such a script file.
+///
+/// Besides checking that the file exists, require that it has an extension:


who does require?

I reworded it to "we require", with "we" being ROOT.

dpiparo · 2026-07-11T05:48:56Z

-         return 0;
-      }
+      Error("DrawSelect",
+            "Drawing using a C++ file currently requires that both the expression and the selection are "


c++ file --> c++ macro?

Good idea! That fits much better

dpiparo

I think the opinion of @pcanal could be useful, however, for me the PR is good to go.

jblomer

LGTM, I think @pcanal should take a look, too.

jblomer · 2026-07-24T13:59:03Z

+      return false;
+
+   const TString candidate = expression;
+   if (candidate.Index("Alt$") >= 0 || candidate.Index("Entries$") >= 0 || candidate.Index("LocalEntries$") >= 0 ||


I think it would be nicer to read to put all the special functions in a vector and then to loop over the vector.

The updated PR is doing that

jblomer · 2026-07-24T14:01:12Z

+// the leading part of a formula (e.g. a file named "abs") must not make
+// TTree::Draw interpret the formula as the name of a C++ script called with
+// arguments: only files with an extension qualify as scripts.
+TEST(TTreeDrawRegressions, ExpressionNotShadowedByFile)


Perhaps also test that the intended behavior still works, i.e. when you leave, say, abs.C as a macro and try to run it.

Added a test for that, tracing the minimal required code path that is required to test with because full ACLiC would take about 10 seconds.

`TTree::Draw` can take the name of a C++ script file instead of a TTreeFormula expression for both the variable expression and the selection. To decide between the two, `TTreePlayer::DrawSelect` checked whether a matching file exists via `TSystem::IsFileInIncludePath()`, which strips a trailing parenthesized part as ACLiC-style macro arguments. A formula like `"abs(Bs_TRUEID)"` was thus reduced to `"abs"`, and if an unrelated file with that name happened to exist in the current directory, the Draw call failed: ``` root [0] tree->Draw("Bs_MCORR", "abs(Bs_TRUEID)") Error in <TTreePlayer::DrawSelect>: Drawing using a C++ macro currently requires that both the expression and the selection are files: "Bs_MCORR" is not a file ``` Two inconsistencies made the trap wider: the selection branch did not require a dot in the candidate string at all, and the varexp branch checked for a dot before stripping the arguments, so a decimal constant inside the parentheses (e.g. `"abs(x+0.5)"`) satisfied it. Factor the decision into a single helper used for both arguments, which requires the candidate - after stripping the ACLiC mode and arguments - to have an extension. This is not a new restriction: `TTreeProxyGenerator::WriteProxy()` already refuses a script without an extension (the function name is derived from the basename minus the extension, for the cut script too), so such a file could never be processed anyway. It now evaluates as a formula instead of failing with a misleading error. Add regression tests that exercise the previously failing Draw calls in the presence of a shadowing file named "abs", and that check a real "abs.C" macro is still recognized as a script because of its extension. Fixes https://its.cern.ch/jira/browse/ROOT-8000 🤖 Done with the help of AI

guitargeek · 2026-07-28T13:02:31Z

@dpiparo and @jblomer, thanks for the review comments! I have addressed them.

@pcanal, you have something to add to the review by Danilo and Jakob?

guitargeek self-assigned this Jul 8, 2026

guitargeek requested review from jblomer and pcanal as code owners July 8, 2026 08:39

guitargeek added bug in:TTree AI-Assisted labels Jul 8, 2026

dpiparo reviewed Jul 11, 2026

View reviewed changes

Comment thread tree/treeplayer/src/TTreePlayer.cxx Outdated

dpiparo reviewed Jul 11, 2026

View reviewed changes

dpiparo approved these changes Jul 11, 2026

View reviewed changes

jblomer approved these changes Jul 24, 2026

View reviewed changes

guitargeek force-pushed the ROOT-8000 branch from 5fc9c32 to 3a22c18 Compare July 28, 2026 07:58

guitargeek force-pushed the ROOT-8000 branch from 3a22c18 to f7edbf1 Compare July 28, 2026 09:11

Uh oh!

Conversation

guitargeek commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dpiparo left a comment

Choose a reason for hiding this comment

Uh oh!

jblomer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guitargeek commented Jul 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

guitargeek commented Jul 8, 2026 •

edited

Loading

github-actions Bot commented Jul 8, 2026 •

edited

Loading