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

interpret Expr(:isdefined, ::QuoteNode) correctly #40562

Closed
wants to merge 1 commit into from

Conversation

simeonschaub
Copy link
Member

@simeonschaub simeonschaub added backport 1.6 Change should be backported to release-1.6 bugfix This change fixes an existing bug compiler:interpreter labels Apr 22, 2021
@simeonschaub simeonschaub force-pushed the sds/interpret_isdefined_quotenode branch from 3c4fa1e to 004bf08 Compare April 22, 2021 11:26
@vtjnash
Copy link
Member

vtjnash commented Apr 22, 2021

This sounds like someone generated corrupt IR, which is not permitted. Using lowering from an AST should prevent this from reaching the backend, and any compiler passes in Core.Compiler are written to avoid generating it later.

Can you add a test to julia/base/compiler/validation.jl instead to reject this?

@vtjnash vtjnash added invalid Indicates that an issue or pull request is no longer relevant compiler:lowering Syntax lowering (compiler front end, 2nd stage) and removed backport 1.6 Change should be backported to release-1.6 bugfix This change fixes an existing bug compiler:interpreter labels Apr 22, 2021
@simeonschaub
Copy link
Member Author

Would it be problematic to consider this valid? Many Cassette style code transformations use Meta.partially_inline! to replace slots or static parameters with QuoteNodes, the specific example here was Zygote: JuliaDebug/JuliaInterpreter.jl#476.

@vtjnash
Copy link
Member

vtjnash commented Apr 22, 2021

Hm, perhaps we should eliminate that code, but in the meantime, it should be taught how to correctly inline a :isdefined node

@JeffBezanson
Copy link
Member

Right; inlining into isdefined is not valid since isdefined is not referentially transparent --- it operates on a variable, not its value, so you can't replace the variable with its value.

simeonschaub added a commit that referenced this pull request Apr 27, 2021
@vtjnash vtjnash deleted the sds/interpret_isdefined_quotenode branch April 27, 2021 14:43
simeonschaub added a commit that referenced this pull request Apr 30, 2021
simeonschaub added a commit that referenced this pull request Jul 1, 2021
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
KristofferC pushed a commit that referenced this pull request Jul 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:lowering Syntax lowering (compiler front end, 2nd stage) invalid Indicates that an issue or pull request is no longer relevant
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants