Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make overriding closure methods an error #39498

Closed
wants to merge 1 commit into from

Conversation

c42f
Copy link
Member

@c42f c42f commented Feb 3, 2021

Overriding closure methods is a strong sign that the programmer is
confused about the lexical scoping rules for closure methods. (That is,
they expect them to behave like generic functions in top level code).
Make this an error so that people can't get caught out by this.

Helps with #15602. May close it, though we may also want detection in lowering as a more complete solution?

After changing this there were two failing tests so this is likely to be slightly breaking for some user code as well. In particular, @testset puts code inside a let block (#33864) so functions declared inside @testset become closures.

Nevertheless, Julia 1.6 will make this a warning due to the changes in #36609 so people will have some time to adjust.

Overriding closure methods is a strong sign that the programmer is
confused about the lexical scoping rules for closure methods. (That is,
they expect them to behave like generic functions in top level code).
Make this an error so that people can't get caught out by this.
@JeffBezanson
Copy link
Member

This doesn't do too much more for #15602, since it can still be a problem even if the signatures are different, e.g.

function f()
    g() = 0
    if cond
        g(x) = 1
    end
end

@c42f
Copy link
Member Author

c42f commented Feb 4, 2021

This doesn't do too much more for #15602, since it can still be a problem even if the signatures are different

Yes, that's true, we really need some scope analysis to sort that out.

Also I've realized there's a problem with this PR in regard to top level let blocks — the following is safe and behaves as expected, but this PR makes it an error following the warning from #33864. So it seems the anon detection based on the current name mangling isn't sufficient.

Current master also has this problem in a lesser way by displaying a warning for this which can't be silenced.

julia> let
           f() = 1
           @show f()
           f() = 2
           @show f()
       end
f() = 1
f() = 2

The macos test failure is some timing issue apparently unrelated to this PR -

channels                           (2) |         failed at 2021-02-03T04:27:43.122
Test Failed at /Users/julia/buildbot/worker-tabularasa/tester_macos64/build/share/julia/test/channels.jl:297
  Expression: ≈(duration, 1, atol = 0.4)
   Evaluated: 1.696307169 ≈ 1 (atol=0.4)

@vtjnash vtjnash closed this Feb 10, 2021
@vtjnash vtjnash deleted the cjf/closure-method-overrides-disabled branch February 10, 2021 04:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants