Make `remove_linenums!` more precise: remove lines from full Expr (was missing macrocalls), but _NOT_ from user's code (by preventing recursion into `esc()`) #31335

NHDaly · 2019-03-13T21:58:49Z

Fix the @inferred and @test macros to stop modifying user inputs.

After several edits, this PR achieves that by making Base.remove_linenums! not recurse into esc()'d Exprs.

This PR also merges in #29619, which strengthens remove_linenums! to also remove linenumbers from macro calls.

Finally, it introduces a new comparison function, Base. isequal_nolines(a, b), which strips lineinfo nodes from both ASTs and then compares them for equality.

Fixes #31334

@macro

Currently `Base.remove_linenums!` removes line information from code blocks, but not from macro calls, which embed line information as the second argument in the expression. Having the line information in there makes it difficult to test user code that performs transformations on expressions, since the line information may differ in the input and resulting expressions. This change extends `Base.remove_linenums!` to handle macro call expressions, replacing their line information with `nothing`. The result is: ```julia julia> :(@macro argument) :(#= REPL[10]:1 =# @macro argument) julia> Base.remove_linenums!(:(@macro argument)) :(@macro argument) ```

NHDaly · 2019-03-13T22:00:32Z

~~@vtjnash's suggestion here is probably better than what I did here.~~

~~... that said, I have always wanted something like this function for recursively replacing part of an AST that matches a pattern. But it's probably not needed for this problem.~~

EDIT: That solution did not work. This PR has been updated.

…red`

…, unclear how to fix kwargs case

This makes sense because the code we introduced that we want to strip linenumbers from and the user's code we want to `esc()` are complementary. - The reason we use `Base.remove_linenums!` is to avoid injecting the linenumbers of the Test code into the user's test expressions. So we want to remove linenums from any code _iff_ it was introduced by _us_ and not the user. - Conversely, we want to always `esc` _any code provided by the user_ exactly once, and we _don't_ want to `esc()` our code, since we want our variable names to get gensym'd.

NHDaly · 2019-09-03T02:39:09Z

The trouble now is that all sorts of tests that compare Exprs will fail, since @test used to strip linenumbers and now it doesn't. Example:

julia/test/syntax.jl

Lines 46 to 47 in 445f216

    
           @test :(module $a end) == :(module a 
        
                                       end)

Test Failed at /Users/sabae/buildbot/worker-tabularasa/tester_macos64/build/share/julia/test/syntax.jl:46
  Expression: $(Expr(:quote, :(module $(Expr(:$, :a))
  #= /Users/sabae/buildbot/worker-tabularasa/tester_macos64/build/share/julia/test/syntax.jl:46 =#
  #= /Users/sabae/buildbot/worker-tabularasa/tester_macos64/build/share/julia/test/syntax.jl:46 =#
  end))) == $(Expr(:quote, :(module a
  #= /Users/sabae/buildbot/worker-tabularasa/tester_macos64/build/share/julia/test/syntax.jl:46 =#
  #= /Users/sabae/buildbot/worker-tabularasa/tester_macos64/build/share/julia/test/syntax.jl:47 =#
  end)))

Can we make this kind of change and just require callers to call Base.remove_linenumbers! explicitly on their expressions before testing them? (Or maybe we can make a (shorter-named?) function that doesn't modify inputs. Maybe strip_linenumbers or without_linenumbers or something?)

It's a breaking change, but only breaking tests, which is maybe easier? It's maybe the kind of breaking where no one realllly minds, and we can just change it? (that is, I'm not sure if we actually promised not to change ASTs, and this feels kinda similar?)

The other option seems to me to somehow either change the definition of ==(::Expr, ::Expr) to remove linenums from both before comparing (Yikes!), or add another comparison function like isapprox(::Expr, ::Expr) that does that.

KristofferC · 2019-09-03T13:12:44Z

or add another comparison function like isapprox(::Expr, ::Expr) that does that.

FWIW, I like this.

NHDaly · 2019-09-13T00:18:10Z

or add another comparison function like isapprox(::Expr, ::Expr) that does that.

FWIW, I like this.

Same... Any suggestions on the name? isapprox doesn't seem quite right..

Adds and exports `isequal_nolines` to `Test` stdlib.

…ed name seems just fine.

…y_input

This will protect user macros that expect to find a LineNumberNode in the macrocall AST

DilumAluthge · 2019-09-13T19:56:07Z

Honestly, I like the idea of just defining isapprox(::Expr, ::Expr).

You can add a new function called something like compare_expr.

But if you use isapprox, you can use the Unicode ≈.

NHDaly · 2019-09-13T20:19:45Z

Thanks Dilum! :)

Actually, after thinking about it more, this function should be for parsing ASTs, which might be expressions or might be literal values, so we should use a function that could mean something else in the case of a literal.

(Note that :(5.0) === 5.0, so we should avoid something like isapprox(:(5.0), :(5.0001)), which presumably the user would want to find out about.)

So i think this should be its own, Expr-tree-/AST- specific function.

I pushed up some commits that implements this and calls it Test.isequal_nolines, which seems explicit/clear enough. I'm also fine with compare_expr or compare_expr_nolines or compare_ast_nolines? I kind of like reusing the is-blank format that isequal and isapprox follow, but i'm happy with whatever everyone can agree on. :)

DilumAluthge · 2019-09-14T00:45:13Z

isequal_nolines seems fine to me. I like compare_expr or compare_ast a little bit better because they are shorter. But you make a good point about isequal_nolines being consist with isequal and isapprox.

You could also do isequal_ast, which is shorter and maintans the consistency.

NHDaly · 2019-09-17T16:16:23Z

(I have merged in #29619, as well, which makes remove_linenums! also strip linenumbers from macrocalls, which was missing before, by replacing the line info node with #= none:1 =#.)

NHDaly · 2019-09-17T16:17:00Z

Please take a look, I think this PR is now ready to be reviewed.

(The existing test failure appears to be a flake.)

Thanks! :)

JeffBezanson · 2019-09-17T20:08:33Z

stdlib/Test/src/Test.jl

+See also: [`isapprox`](@ref) for _numerical_ approximate equality
+"""
+function isequal_nolines(a, b)
+    Base.remove_linenums!(a) == Base.remove_linenums!(b)


This should copy its arguments so the predicate overall is non-mutating.

Oops, duh, thanks.

deepcopy() or copy()? I feel like deepcopy, but i still don't really understand the difference b/w them.

copy on a container makes a new container with the same elements. So copy of an array allocates exactly one new array. Exprs are more of a tree, so it copies the tree structure (Exprs and all sub-Exprs) but keeps the "leaves". So I think copy is correct here.

deepcopy attempts to copy everything reachable from an object, as if you serialized the whole thing to disk and read it back in. deepcopy is best avoided, since it violates abstraction.

Suggested change

Base.remove_linenums!(a) == Base.remove_linenums!(b)

a = Base.remove_linenums!(copy(a))

b = Base.remove_linenums!(copy(b))

return a == b

JeffBezanson · 2019-09-17T20:17:09Z

I don't think excluding :escape expressions is fully correct, it just happens to work in this case. esc is just a scope marker, and it's likely we'll change how hygiene works in the future not to use it.

Did you try simply removing the remove_linenums! calls from Test and seeing if there are any ill effects? I'm hoping that will work now.

StefanKarpinski · 2019-09-30T22:03:46Z

Bump.

vtjnash · 2019-10-01T00:38:07Z

stdlib/Test/src/Test.jl

+See also: [`isapprox`](@ref) for _numerical_ approximate equality
+"""
+function isequal_nolines(a, b)
+    Base.remove_linenums!(a) == Base.remove_linenums!(b)


Suggested change

Base.remove_linenums!(a) == Base.remove_linenums!(b)

a = Base.remove_linenums!(copy(a))

b = Base.remove_linenums!(copy(b))

return a == b

vtjnash · 2019-10-01T00:41:46Z

base/expr.jl

+        # Replace line information embedded into macro calls with `nothing`
+        # Removing the argument entirely invalidates the expression
+        map!(ex.args, ex.args) do arg
+            arg isa LineNumberNode ? LineNumberNode(1, :none) : remove_linenums!(arg)


These seems controversial, since these are values, not line numbers it’s removing. We could strip the one from the first argument (replacing __source__ with nothing), but I don’t think we should modify the other argument literals in the list.

MilesCranmer · 2024-05-28T11:08:39Z

Friendly ping

NHDaly force-pushed the inferred_dont_modify_input branch from 487b92d to e8585bc Compare August 27, 2019 15:35

NHDaly added 7 commits August 27, 2019 11:44

Add failing tests showing the broken behavior for @test and `@infer…

6a39215

…red`

Fix at-inferred to not modify inputs

5a8d411

Wrap tests in Testset

641fb2f

Add another test for at-inferred w/ kwargs

cae4d35

try switching to replace_subtree! implementation; fixes simple case…

f83317f

…, unclear how to fix kwargs case

Revert src changes in this PR; tests are all broken again

4e77618

NHDaly force-pushed the inferred_dont_modify_input branch from e8585bc to 445f216 Compare August 27, 2019 16:40

NHDaly changed the title ~~Add Base.replace_subtree!; fix @inferred Expr inputs~~ Fix at-inferred and at-test macros to not strip linenumbers from input Expressions. Aug 27, 2019

NHDaly changed the title ~~Fix at-inferred and at-test macros to not strip linenumbers from input Expressions.~~ Fix @inferred and @test macros to not strip linenumbers from input Expressions. Aug 27, 2019

NHDaly changed the title ~~Fix @inferred and @test macros to not strip linenumbers from input Expressions.~~ Fix @inferred and @test macros to not strip linenumbers from input Expressions: Stop remove_linenums! from recursing into esc() Aug 28, 2019

NHDaly mentioned this pull request Aug 29, 2019

inferred macro causes :block Expr arguments to lose LineNumberNodes #31334

Open

NHDaly added 4 commits September 13, 2019 08:18

Add Test.isequal_nolines: remove_linenums! then isequal.

c740288

Adds and exports `isequal_nolines` to `Test` stdlib.

Un-export Test.isequal_nolines: no reason to export it, the qualifi…

4a0c48f

…ed name seems just fine.

Use Test.isequal_nolines to fix julia tests comparing Exprs for equality

1795d51

Fix isequal_nolines docstring: remove line nums from docstring

3143b88

NHDaly force-pushed the inferred_dont_modify_input branch from 9560559 to 3143b88 Compare September 13, 2019 13:55

NHDaly added 3 commits September 13, 2019 15:43

Merge branch 'aa/aggressive-remove_linenums' into inferred_dont_modif…

2ce69bc

…y_input

Merge branch 'master' into inferred_dont_modify_input

a6a4560

Change @ararslan's remove-lines to replace w/ none:1

ec73140

This will protect user macros that expect to find a LineNumberNode in the macrocall AST

NHDaly mentioned this pull request Sep 17, 2019

Remove line info from macro calls in remove_linenums! #29619

Closed

JeffBezanson reviewed Sep 17, 2019

View reviewed changes

vtjnash reviewed Oct 1, 2019

View reviewed changes

vtjnash mentioned this pull request May 29, 2021

Document renumbering lines in macros #40955

Closed

Liozou mentioned this pull request Dec 21, 2021

Unit testing: @test_throws regression: throw is not anymore detected #43289

Closed

NHDaly mentioned this pull request Jan 3, 2023

Set line info for @client macro as call site JuliaWeb/HTTP.jl#981

Merged

ericphanson mentioned this pull request May 28, 2024

Better source tracking MilesCranmer/DispatchDoctor.jl#7

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `remove_linenums!` more precise: remove lines from full Expr (was missing macrocalls), but _NOT_ from user's code (by preventing recursion into `esc()`) #31335

Make `remove_linenums!` more precise: remove lines from full Expr (was missing macrocalls), but _NOT_ from user's code (by preventing recursion into `esc()`) #31335

NHDaly commented Mar 13, 2019 •

edited

NHDaly commented Mar 13, 2019 •

edited

NHDaly commented Sep 3, 2019

KristofferC commented Sep 3, 2019

NHDaly commented Sep 13, 2019

DilumAluthge commented Sep 13, 2019

NHDaly commented Sep 13, 2019

DilumAluthge commented Sep 14, 2019

NHDaly commented Sep 17, 2019

NHDaly commented Sep 17, 2019

JeffBezanson Sep 17, 2019

NHDaly Sep 17, 2019

JeffBezanson Sep 17, 2019

vtjnash Oct 1, 2019

JeffBezanson commented Sep 17, 2019

StefanKarpinski commented Sep 30, 2019

vtjnash Oct 1, 2019

vtjnash Oct 1, 2019

MilesCranmer commented May 28, 2024

-    Base.remove_linenums!(a) == Base.remove_linenums!(b)
+    a = Base.remove_linenums!(copy(a))
+    b = Base.remove_linenums!(copy(b))
+    return a == b

Make remove_linenums! more precise: remove lines from full Expr (was missing macrocalls), but _NOT_ from user's code (by preventing recursion into esc()) #31335

Are you sure you want to change the base?

Make remove_linenums! more precise: remove lines from full Expr (was missing macrocalls), but _NOT_ from user's code (by preventing recursion into esc()) #31335

Conversation

NHDaly commented Mar 13, 2019 • edited

NHDaly commented Mar 13, 2019 • edited

NHDaly commented Sep 3, 2019

KristofferC commented Sep 3, 2019

NHDaly commented Sep 13, 2019

DilumAluthge commented Sep 13, 2019

NHDaly commented Sep 13, 2019

DilumAluthge commented Sep 14, 2019

NHDaly commented Sep 17, 2019

NHDaly commented Sep 17, 2019

JeffBezanson Sep 17, 2019

Choose a reason for hiding this comment

NHDaly Sep 17, 2019

Choose a reason for hiding this comment

JeffBezanson Sep 17, 2019

Choose a reason for hiding this comment

vtjnash Oct 1, 2019

Choose a reason for hiding this comment

JeffBezanson commented Sep 17, 2019

StefanKarpinski commented Sep 30, 2019

vtjnash Oct 1, 2019

Choose a reason for hiding this comment

vtjnash Oct 1, 2019

Choose a reason for hiding this comment

MilesCranmer commented May 28, 2024

Make `remove_linenums!` more precise: remove lines from full Expr (was missing macrocalls), but _NOT_ from user's code (by preventing recursion into `esc()`) #31335

Make `remove_linenums!` more precise: remove lines from full Expr (was missing macrocalls), but _NOT_ from user's code (by preventing recursion into `esc()`) #31335

NHDaly commented Mar 13, 2019 •

edited

NHDaly commented Mar 13, 2019 •

edited