Repository navigation
Conversation
parseStatement returned null for those paths; mixin semantic then dereferenced s.loc in errorSupplemental. Return ErrorStatement instead and guard the supplemental location. Fixes dlang#23464 Co-authored-by: Cursor <cursoragent@cursor.com>
1d9a147 to
5adb32a
Compare
parse.d now uses AST.ErrorStatement.get(), which only existed in the
full AST, so Parser!ASTBase (e.g. compiler/test/dub_package/avg.d and
impvisitor.d) no longer compiled. Add the same get() to ASTBase. The
singleton is created lazily, as ASTBase.ErrorExp.get() does, because
the constructor's `global.errors || global.gaggedErrors` assert cannot
run at compile time.
Only the `version =` / `debug =` paths need to return an ErrorStatement,
and both always report an error first. Restore the original `s = null`
for a failed `if` statement, which the fix does not need, and drop the
extra comments.
Extend the test to `debug = ident;` and to `mixin("else")`, whose
supplemental message previously had no location.
| auto eSink = global.errorSink; | ||
| eSink.errorSupplemental(s.loc, "while parsing string mixin statement"); | ||
| // s may be null or an ErrorStatement, which has no location | ||
| const loc = s && s.loc.isValid() ? s.loc : cs.loc; |
| { | ||
| auto eSink = global.errorSink; | ||
| eSink.errorSupplemental(s.loc, "while parsing string mixin statement"); | ||
| // s may be null or an ErrorStatement, which has no location |
There was a problem hiding this comment.
(Repositing as GitHub has been bugging out on my last comment) Should clarify when s is null, and why it's not consistently null or consistently ErrorStatement
There was a problem hiding this comment.
Thanks, good catches. parseStatement returns null on some error paths (version =, debug =, a failed if, and ident::, which also crashed) and an ErrorStatement with no location on others, so trying to make the parser consistent kept missing cases. I went the other way: the parser changes are reverted, and the mixin code no longer looks at s. It always puts "while parsing string mixin statement" on the mixin's own location, so there's no s.loc left and nothing that needs a comment. Dropping ErrorStatement.get() from the parser also fixes the GDC 9 build, which couldn't see it through ASTCodegen. The new test covers all of those cases, and four existing tests now expect the note on the mixin line.
I used AI for these changes and this comment; if anything smells fishy, ping me and I'll take a closer look.
There was a problem hiding this comment.
@AMDphreak Please post human written comments
What are you talking about? Code comments or PR comments?
There was a problem hiding this comment.
In general both, but in this case it was regarding the response to my review comments. The AI has a lot to say about what it does but now why it does it, so I'm asking for a human explanation showing intent and understanding about the choice between s.loc and cs.loc, and ErrorStatement and null.
parseStatement returns null on some error paths (`version =` and
`debug =` inside a function, a failed `if`, `ident::`) and an
ErrorStatement without a location on others. Rather than trying to make
the parser consistent, report "while parsing string mixin statement" at
the mixin expression's location (cs.loc) and never look at `s`.
This also fixes `mixin("a::b;")`, which still crashed, and lets us drop
the parser/astbase ErrorStatement.get() changes from the previous
commit. Those broke the GDC 9 host build ("no property 'ErrorStatement'
for type 'ASTCodegen'"), because older frontends can't resolve a static
member through ASTCodegen's public imports without an explicit alias.
The supplemental in fail246, fail265, fail4375k and ice8795 now points
at the mixin's line in the source file. The new test gains the `a::b`
and `else` cases.
AMDphreak
changed the title
version=/debug= statements
3 participants
Summary
version =/debug =(and similar parse paths that returned a nullStatement).parseStatementnow returnsErrorStatementinstead ofnull; mixin semantic null-guards the supplemental location.fail_compilation/mixin_version_debug_assign.d.Fixes #23464
Test plan
mixin("version = foo;")/mixin("debug = 1;")(exit-1073741819/0xC0000005)1with normal diagnosticsmixin_version_debug_assign.d