Apply suggestions from code review

Co-authored-by: Claire Foster <[email protected]>

Author: mlechu

src/desugaring.jl

@@ -1357,7 +1357,9 @@ function expand_assignment(ctx, ex, is_const=false)
             # Identifier in lhs[1] is a variable type declaration, eg
             # x::T = rhs
             @ast ctx ex [K"block"
-                kind(x) === K"Placeholder" ? nothing : [K"decl" x T]
+                if kind(x) !== K"Placeholder"
+                     [K"decl" x T]
+                end
                 is_const ? [K"const" [K"=" x rhs]] : [K"=" x rhs]
             ]
         else
@@ -2172,8 +2174,12 @@ function make_lhs_decls(ctx, stmts, declkind, declmeta, ex, type_decls=true)
             if kind(name) == K"Identifier"
                 push!(stmts, makenode(ctx, ex, K"decl", name, ex[2]))
             else
-                # Setting the global type of underscores is likely undesired.
-                # Locals are debatable, though that isn't known here.
+                # TODO: Currently, this treats both globals and locals the same
+                # way by ignoring the LHS in `_::T = val`.  For locals, we
+                # should probably do one of the following:
+                # - Throw a LoweringError if that's not too breaking
+                # - `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"
                 return
             end
@@ -2316,11 +2322,14 @@ function expand_function_arg(ctx, body_stmts, arg, is_last_arg, is_kw, arg_id)
             [K"=" ex name]
         ])
     elseif k == K"Placeholder"
-        # The user shouldn't be able to access this name, but lowering should be
-        # able to use it as an rvalue internally, e.g. for kw method dispatch.
-        # Duplicate positional placeholder names should be allowed.
-        name_str = is_kw ? "#kw$(ex.name_val)#" : "#arg$(string(arg_id))#"
-        name = @ast ctx ex name_str::K"Identifier"
+        # Lowering should be able to use placeholder args as rvalues internally,
+        # e.g. for kw method dispatch.  Duplicate positional placeholder names
+        # should be allowed.
+        name = if is_kw
+            @ast ctx ex ex=>K"Identifier"
+        else
+            new_local_binding(ctx, ex, "#arg$(string(arg_id))#"; kind=:argument)
+        end
     elseif k == K"Identifier"
         name = ex
     else

src/macro_expansion.jl

@@ -382,16 +382,15 @@ function expand_forms_1(ctx::MacroExpansionContext, ex::SyntaxTree)
     k = kind(ex)
     if k == K"Identifier"
         name_str = ex.name_val
-        if all(==('_'), name_str)
-            @ast ctx ex ex=>K"Placeholder"
-        elseif is_ccall_or_cglobal(name_str)
+        if is_ccall_or_cglobal(name_str)
             # Lower special identifiers `cglobal` and `ccall` to `K"core"`
             # pseudo-refs very early so that cglobal and ccall can never be
             # turned into normal bindings (eg, assigned to)
             @ast ctx ex name_str::K"core"
         else
-            layerid = get(ex, :scope_layer, current_layer_id(ctx))
-            makeleaf(ctx, ex, ex, kind=K"Identifier", scope_layer=layerid)
+            k = all(==('_'), name_str) ? K"Placeholder" : K"Identifier"
+            scope_layer = get(ex, :scope_layer, current_layer_id(ctx))
+            makeleaf(ctx, ex, ex; kind=k, scope_layer)
         end
     elseif k == K"StrMacroName" || k == K"CmdMacroName" || k == K"macro_name"
         # These can appear outside of a macrocall, e.g. in `import`

src/runtime.jl

@@ -337,10 +337,9 @@ function (g::GeneratedFunctionStub)(world::UInt, source::Method, @nospecialize a
 
     __module__ = source.module
 
-    # Macro expansion. Looking at Core.GeneratedFunctionStub, it seems that
-    # macros emitted by the generator are currently expanded in the latest
-    # world, so do that for compatibility.
-    macro_world = typemax(UInt)
+    # Macro expansion. Note that we expand in `tls_world_age()` (see
+    # Core.GeneratedFunctionStub)
+    macro_world = Base.tls_world_age()
     ctx1 = MacroExpansionContext(graph, __module__, false, macro_world)
 
     layer = only(ctx1.scope_layers)