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.

Sign up for GitHub

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

Jump to bottom

Follow up #54772 - don't accidentally put Module into method name slot #54856

Merged
merged 1 commit into from
Jun 23, 2024
Merged

Follow up #54772 - don't accidentally put Module into method name slot #54856

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Follow up #54772 - don't accidentally put Module into method name slot 
The `:outerref` removal (#54772) ended up accidentally putting a `Module`
argument into 3-argument `:method` due to a bad refactor. We didn't catch
this in the tests, because the name slot of 3-argument `:method` is
 unused (except for external method tables), but ordinarily contains
the same data as 1-argument `:method`.
That said, some packages in the Revise universe look at this
(arguably incorrectly, since they should be looking at the
signature instead), so it should be correct until we fix Revise,
at which point we may just want to always pass `false` here.
  • Loading branch information
@Keno
Keno committed Jun 23, 2024
commit 5262090ded02c4eab54982ccd2ba4a37e22e672a
8 changes: 3 additions & 5 deletions src/julia-syntax.scm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,10 +227,8 @@

(define (method-expr-name m)
(let ((name (cadr m)))
(let ((name (if (or (length= m 2) (not (pair? name)) (not (quoted? name))) name (cadr name))))
(cond ((not (pair? name)) name)
((eq? (car name) 'globalref) (caddr name))
(else name)))))
(cond ((globalref? name) (caddr name))
(else name))))

;; extract static parameter names from a (method ...) expression
(define (method-expr-static-parameters m)
Expand Down Expand Up @@ -4087,7 +4085,7 @@ f(x) = yt(x)
(newlam (compact-and-renumber (linearize (car exprs)) 'none 0)))
`(toplevel-butfirst
(block ,@sp-inits
(method ,name ,(cl-convert sig fname lam namemap defined toplevel interp opaq globals locals)
(method ,(cadr e) ,(cl-convert sig fname lam namemap defined toplevel interp opaq globals locals)
vtjnash marked this conversation as resolved.
Show resolved Hide resolved
,(julia-bq-macro newlam)))
,@top-stmts))))

Expand Down
13 changes: 13 additions & 0 deletions test/syntax.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3845,3 +3845,16 @@ struct SingletonMaker; end
const no_really_this_is_a_function_i_promise = Val{SingletonMaker()}()
no_really_this_is_a_function_i_promise(a) = 2 + a
@test Val{SingletonMaker()}()(2) == 4

# Test that lowering doesn't accidentally put a `Module` in the Method name slot
let src = @Meta.lower let capture=1
global foo_lower_block
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, the reason this must be in lowering (and wrong to inspect the signature unless lowering instructs that it must) is that this line could be:

Suggested change
global foo_lower_block
global const foo_lower_block = Base.redirect_stdin

And looking at the signature would wrongly name this Method as Base.RedirectStdStream(0), when syntactically it has the different intended name foo_lower_block

foo_lower_block() = capture
end
code = src.args[1].code
for i = length(code):-1:1
expr = code[i]
Meta.isexpr(expr, :method) || continue
@test isa(expr.args[1], Union{GlobalRef, Symbol})
end
end