Track params in the parser #4777

geoffromer · 2025-01-08T22:26:50Z

This change splits NodeKind::IdentifierName into separate node kinds depending on whether the identifier is followed by parameters, and similarly splits NameQualifier based on whether the qualifier has parameters. This enables us to only push a pattern block when it's actually needed, rather than "defensively" pushing one when it might be needed.

jonmeow · 2025-01-09T00:19:25Z

Just glancing, did you notice this is regressing the diagnostic location? i.e., before it pointed at parameters:

// CHECK:STDERR: fail_params.carbon:[[@LINE+7]]:8: error: `alias` declaration cannot have parameters [UnexpectedDeclNameParams]
// CHECK:STDERR: alias A(T:! type) = T*;
// CHECK:STDERR:        ^~~~~~~~~~

Now it points at the node after the parameters (and doesn't explain why this name can't have parameters):

// CHECK:STDERR: fail_params.carbon:[[@LINE+3]]:19: error: unexpected parameters after name declaration [UnexpectedParamsAfterDeclName]
// CHECK:STDERR: alias A(T:! type) = T*;
// CHECK:STDERR:                   ^

Also, the PR description is empty, should it have a little more detail about what's changing (and motivation)? e.g., it looks like you're adding node kinds, maybe the PR could explain the approach?

jonmeow · 2025-01-09T00:41:15Z

I should also note, #3988 had moved validation from parse to check in order to get more uniform handling. It might be good to explain how the changes there are adapted here in that context, particularly since this is changing diagnostics (are incorrectly placed parameters ever diagnosed in check after this change?)

edit: something like fn NS(x: I32).F() probably needs to be handled in check. But do you have tests for something like

// This needs to be handled in check.
fn NS(x: i32).F() {}

// And we'll probably have static vars, like:
class C(T:! type) {
  static var x: i32;
}
var C(T:! type).x = i32;

// Is the parse approach going to be able to reject incorrect params in this syntax?
var C(T:! type).x() = i32;

(also, alias may get params, I think that's not yet truly decided)

danakj

Thanks for looking as well Jon, this is definitely a challenging review for my first auto-assignment 😆

The IdentifierNameNotBeforeParams and IdentifierNameBeforeParams change seems nice, in that its making check a little more straightforward. Given the convo above, I am not sure about the DeclNameAndParams part of the change though, as it seems to be limiting things a bit more than it should? Are these two tired together necessarily? That's hard for me to tell, but it might be nice to just do the IdentifierNameNotBeforeParams part of the PR independently, if that doesn't change how diagnostics are being reported, and then it'll be easier to think about the other part.

toolchain/check/name_component.cpp

toolchain/parse/state.def

jonmeow · 2025-01-09T16:42:53Z

np, happy to look.

Also, @geoffromer, I think you need to re-add tests of how this behaves in check. The parse tree still contains the pattern, so it needs to be correctly handled (i.e., as currently implemented, the pattern block still neds to be pushed). I'm actually not sure that this change is removing the need for any of the work being done in check. (it may be that the TODOs are incorrect as a consequence -- the logic in check may need to stay)

toolchain/check/name_component.cpp

jonmeow · 2025-01-09T17:17:57Z

I'm actually not sure that this change is removing the need for any of the work being done in check. (it may be that the TODOs are incorrect as a consequence -- the logic in check may need to stay)

Oh, I think I'm understanding part of this now. You're using the different parse node to only push a param block for when there's a pattern. Regardless, I think that still needs to be tested -- we need to make sure we aren't crashing on invalid code.

geoffromer

The IdentifierNameNotBeforeParams and IdentifierNameBeforeParams change seems nice, in that its making check a little more straightforward. Given the convo above, I am not sure about the DeclNameAndParams part of the change though, as it seems to be limiting things a bit more than it should? Are these two tired together necessarily? That's hard for me to tell, but it might be nice to just do the IdentifierNameNotBeforeParams part of the PR independently, if that doesn't change how diagnostics are being reported, and then it'll be easier to think about the other part.

Oh, that's a good idea. Yes, they do turn out to be separable, so I've removed the DeclNameAndParams parts from this PR. I think that makes the other top-level comments moot, but let me know if I've missed anything.

Note that this means that this PR no longer resolves those TODOs as stated, but still removes them. I'm going to seek clarification on Discord about whether the folks who asked for them still want another PR with the parser diagnostic changes.

toolchain/check/name_component.cpp

danakj

A couple questions to help me understand

danakj · 2025-01-10T15:17:10Z

toolchain/parse/typed_nodes.h

+// to be followed by parameters.
+using IdentifierNameBeforeParams =
+    LeafNode<NodeKind::IdentifierNameBeforeParams, Lex::IdentifierTokenIndex,
+             NodeCategory::MemberName | NodeCategory::NonExprIdentifierName>;


How come IdentifierName was not NodeCategory::NonExprIdentifierName before, but this is now?

That category didn't exist before; I introduced it in this PR in order to use AnyNonExprIdentifierName to talk about identifiers when we don't care whether they're followed by parameters.

Ideally I would have called this category IdentifierName, like the thing it's replacing, but my concern was that AnyIdentifierName sounds like it would encompass IdentifierNameExpr, so I went with a more specific name. I also considered adding NonExpr to the names of these two node types, but I just couldn't stomach the verbosity of e.g. NonExprIdentifierNameNotBeforeParams.

FWIW, one thing to mull might be if IdentifierNameExpr could actually be NameRef (since it looks like it's always translated to SemIR::NameRef), which might avoid the ambiguity of this

True, although SemIR::NameRefs can also be created from other node kinds, so they don't line up perfectly. Definitely an option to consider, though.

danakj · 2025-01-10T15:25:21Z

toolchain/parse/typed_nodes_test.cpp

@@ -252,18 +253,16 @@ TEST_F(TypedNodeTest, VerifyExtractTraceClassDecl) {
  EXPECT_THAT(err.message(), testing::MatchesRegex(
                                 R"Trace(Aggregate [^:]*: begin
 Aggregate [^:]*: begin
-Aggregate [^:]*: begin


What makes this go away? (Not really sure what to make of these tests yet..)

Honestly, I have no idea; I don't really understand this test. I just took the failing test result, pasted the actual output back into the code, and replaced the mangled type names with regexes.

I think before, NameAndParams existed as a member aggregate. It's gone without a replacement [ed: not an aggregate one anyways].

jonmeow

Generally looks good, and as mentioned I think it makes sense to keep the param validation in check.

Just some minor questions, but these changes generally look good to me.

toolchain/parse/handle_binding_pattern.cpp

toolchain/parse/typed_nodes.h

toolchain/parse/node_ids.h

toolchain/parse/handle_expr.cpp

toolchain/parse/handle_period.cpp

toolchain/check/name_component.cpp

Co-authored-by: Jon Ross-Perkins <[email protected]>

jonmeow

Thanks, this looks good!

geoffromer added 3 commits January 8, 2025 13:13

Disallow params in the parser, where appropriate

0ffd6d2

Add comments and clean up FIXME

997d6c0

Merge branch 'trunk' into pattern-parse

8a01dea

github-actions bot requested a review from danakj January 8, 2025 22:27

github-actions bot added the toolchain label Jan 8, 2025

Fix up bad merge

1697503

danakj reviewed Jan 9, 2025

View reviewed changes

toolchain/check/name_component.cpp Outdated Show resolved Hide resolved

toolchain/check/name_component.cpp Outdated Show resolved Hide resolved

toolchain/parse/state.def Outdated Show resolved Hide resolved

jonmeow reviewed Jan 9, 2025

View reviewed changes

toolchain/check/name_component.cpp Outdated Show resolved Hide resolved

Revert diagnostic changes

54ec39c

geoffromer changed the title ~~Disallow params in the parser, where appropriate~~ Track params in the parser Jan 9, 2025

Respond to reviewer comments

168f245

geoffromer commented Jan 10, 2025

View reviewed changes

toolchain/check/name_component.cpp Outdated Show resolved Hide resolved

danakj reviewed Jan 10, 2025

View reviewed changes

jonmeow reviewed Jan 10, 2025

View reviewed changes

geoffromer and others added 2 commits January 10, 2025 10:26

Apply suggestions from code review

2d6ea62

Co-authored-by: Jon Ross-Perkins <[email protected]>

Respond to reviewer comments

4883b71

geoffromer requested review from jonmeow and danakj January 10, 2025 19:08

jonmeow approved these changes Jan 10, 2025

View reviewed changes

danakj approved these changes Jan 10, 2025

View reviewed changes

Merge branch 'trunk' into pattern-parse

cdf75a8

geoffromer enabled auto-merge January 10, 2025 20:59

geoffromer added this pull request to the merge queue Jan 10, 2025

Merged via the queue into carbon-language:trunk with commit 4f10735 Jan 10, 2025
8 checks passed

geoffromer deleted the pattern-parse branch January 10, 2025 23:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Track params in the parser #4777

Track params in the parser #4777

geoffromer commented Jan 8, 2025 •

edited

Loading

jonmeow commented Jan 9, 2025 •

edited

Loading

jonmeow commented Jan 9, 2025 •

edited

Loading

danakj left a comment

jonmeow commented Jan 9, 2025 •

edited

Loading

jonmeow commented Jan 9, 2025

geoffromer left a comment

danakj left a comment

danakj Jan 10, 2025

geoffromer Jan 10, 2025

jonmeow Jan 10, 2025

geoffromer Jan 10, 2025

danakj Jan 10, 2025

geoffromer Jan 10, 2025

jonmeow Jan 10, 2025 •

edited

Loading

jonmeow left a comment

jonmeow left a comment

Track params in the parser #4777

Track params in the parser #4777

Conversation

geoffromer commented Jan 8, 2025 • edited Loading

jonmeow commented Jan 9, 2025 • edited Loading

jonmeow commented Jan 9, 2025 • edited Loading

danakj left a comment

Choose a reason for hiding this comment

jonmeow commented Jan 9, 2025 • edited Loading

jonmeow commented Jan 9, 2025

geoffromer left a comment

Choose a reason for hiding this comment

danakj left a comment

Choose a reason for hiding this comment

danakj Jan 10, 2025

Choose a reason for hiding this comment

geoffromer Jan 10, 2025

Choose a reason for hiding this comment

jonmeow Jan 10, 2025

Choose a reason for hiding this comment

geoffromer Jan 10, 2025

Choose a reason for hiding this comment

danakj Jan 10, 2025

Choose a reason for hiding this comment

geoffromer Jan 10, 2025

Choose a reason for hiding this comment

jonmeow Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

jonmeow left a comment

Choose a reason for hiding this comment

jonmeow left a comment

Choose a reason for hiding this comment

geoffromer commented Jan 8, 2025 •

edited

Loading

jonmeow commented Jan 9, 2025 •

edited

Loading

jonmeow commented Jan 9, 2025 •

edited

Loading

jonmeow commented Jan 9, 2025 •

edited

Loading

jonmeow Jan 10, 2025 •

edited

Loading