Lowering bugfix batch 3 #99
Conversation
It looks like the native lowerer drops type information even for local bindings: julia> Meta.@lower let x = (1,2)
_::Float64, y = x
y
end
:($(Expr(:thunk, CodeInfo(
1 ─ %1 = builtin Core.tuple(1, 2)
│ x = %1
│ @ none within `unknown scope`
│ ┌ @ REPL[13]:2 within `macro expansion`
│ │ %3 = x
│ │ %4 = dynamic Base.indexed_iterate(%3, 1)
│ │ builtin Core.getfield(%4, 1)
│ │ #s4 = Core.getfield(%4, 2)
│ │ %7 = x
│ │ %8 = #s4
│ │ %9 = dynamic Base.indexed_iterate(%7, 2, %8)
│ │ y = Core.getfield(%9, 1)
│ │ @ REPL[13]:3 within `macro expansion`
│ │ %11 = y
└──│ return %11
└
))))It's definitely better to drop type information in global scope, I think dropping it in local scope to match that is one option. Personally I'd prefer an error to occur in the above local case though. |
mlechu
force-pushed
the
ec/bugfix
branch
2 times, most recently
from
786e153 to
262e506
Compare
src/desugaring.jl
Outdated
| # Setting the global type of underscores is likely undesired. | ||
| # Locals are debatable, though that isn't known here. | ||
| @chk kind(name) == K"Placeholder" |
There was a problem hiding this comment.
For locals, local x::T = rhs roughly means x = convert(T, rhs)::T so _::T = rhs could possibly mean convert(T, rhs)::T as it can be useful in unpacking argument lists. But really it's a bit strange to support this syntax at all and arguably it should actually be a lowering error as it doesn't do what the user might have intended and is instead elided completely.
I guess we can't get away with throwing LoweringError here because you've seen code actually using this?
| # Setting the global type of underscores is likely undesired. | |
| # Locals are debatable, though that isn't known here. | |
| @chk kind(name) == K"Placeholder" | |
| # TODO: Throw LoweringError here in the future for `_::T = rhs` rather than eliding the left hand side | |
| # (Alternative, do `convert(T, rhs)::T` and discard the result which is what `x::T = rhs` would do if | |
| # x is never used again.) | |
| @chk kind(name) == K"Placeholder" |
There was a problem hiding this comment.
Added the TODO; conversion is probably ideal, but would need extra changes. I'd be OK with trying a LoweringError (the flisp behaviour of local _::T is broken anyway), but we produce our own K"local" declarations from within desugaring in several places, so that would probably need to go in validation.jl.
src/desugaring.jl
Outdated
| name_str = is_kw ? "#kw$(ex.name_val)#" : "#arg$(string(arg_id))#" | ||
| name = @ast ctx ex name_str::K"Identifier" |
There was a problem hiding this comment.
We should make this consistent with optional_positional_defs!() - if you look at the top of that function, you'll see it already has a special case for when K"Placeholder" occurs, in which case it generates a new binding using new_local_binding(...; kind=:argument).
So we should probably just move that code here if we need it in general. We can still use name mangling to construct #arg$arg_id# as a name for debugging purposes, if we want, but the binding ID will be the definitive identity of the introduced local.
One of the goals of JuliaLowering is to entirely remove name mangling or the notion of "compiler internal names" as an important mechanism and replace it with unique binding IDs - then we never need to worry about mangled names colliding regardless of how we use them.
There was a problem hiding this comment.
Also, we introduce a K"Placeholder" further up within expand_function_arg() in case someone writes ::T as an argument - do we need to fix that case too?
There was a problem hiding this comment.
Note also that new_local_binding will set is_internal=true on the binding which is good metadata to have, distinct from relying on name mangling.
We may eventually want another design iteration to smooth out exactly how this all works from desugaring and make it consistent with the other way of introducing named internal variables (ie, K"Identifier" tagged with a new_scope_layer layer ID - which are still currently used for some cases)
There was a problem hiding this comment.
See #106 which enforces a scope layer for new Identifiers emitted by desugaring.
There was a problem hiding this comment.
Changed to use new_local_binding, thanks! ::T placeholders should be handled in the same way as _::T.
We may eventually want another design iteration to smooth out exactly how this all works from desugaring and make it consistent with the other way of introducing named internal variables (ie, K"Identifier" tagged with a new_scope_layer layer ID - which are still currently used for some cases)
Makes sense. Directly producing Bindings from desugaring feels like the best option.
… temporarily rm our `@atomic`
Ensure hasmethod succeeds for ccall
Co-authored-by: Claire Foster <[email protected]>
|
Anything left to do to get this merged? |
Fix placeholders in positional and keyword arguments.
way we desugar keyword functions; give them an internal name. (fix Nameless parameters don't work with kwargs nearby #55, port Allow underscore (unused) args in presence of kwargs JuliaLang/julia#58803)
kws...may use a placeholder (fix var-kwargs with underscore name throws LoweringError #49)I do this by renaming placeholders to an internal name, which feels like a bit
of a hack, but our "duplicate argument name" logic happens in scope resolution
after keywords and non-keywords are mixed.
Allow placeholders in decls:
local/global _,local/global _::T. Thiscan be useful syntax in destructuring assignments. I've implemented this to
always drop the type (addressing Assignment to
_sets the binding type of_JuliaLang/julia#57497),but this may not be what we want in the local case.
Work around
hasmethodalways returning false forworld=typemax(UInt)(somediscussion here). I'm not
sure we should be using
hasmethodin the first place.Comment out our
@atomicimplementation. We need to implement this for allforms that the normal
@atomictakes, otherwise it hijacks the expansion andexpects a simple form.