Parse parameters in name qualifiers. #3988

zygoloid · 2024-05-24T23:38:16Z

Parse the name of a declaration as a sequence of NameQualifiers -- which have a name, possibly parameters, and a trailing period -- followed by a name and possibly parameters. This prepares us for parsing declarations of members of generic classes and similar cases, but actually supporting such member redeclarations is left to a future change.

We previously required functions to have parameters, but no longer do, following the direction of #3848. Cases like namespaces that can't actually have parameters are now diagnosed in check instead of in parse.

Parse the name of a declaration as a sequence of name qualifiers -- which have a name, possibly parameters, and a trailing period -- followed by a name and possibly parameters. Cases like namespaces that can't actually have parameters are now diagnosed in check instead of in parse.

jonmeow

I'm not familiar with #3848, and I'm having trouble loading it because it has merge errors. Does this PR indicate that leads have in effect approved the proposal? Is there some reference for what design you're trying to implement?

toolchain/check/decl_name_stack.cpp

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

zygoloid · 2024-05-28T20:45:39Z

I'm not familiar with #3848, and I'm having trouble loading it because it has merge errors.

I believe the PR is only intending to add its p3848.md file and the rest of the changes listed there are spurious.

Does this PR indicate that leads have in effect approved the proposal?

We've not approved the proposal but we have consensus on the main points (including #3860, which is directly relevant here). But even without #3848, I think it'd still make sense to use the same parse rules (including parameters) for all names, and do any declaration-kind-specific checking in check. The main impact that #3848 has here is that this PR emits a TODO error for parameterless functions, rather than adding a proper diagnostic in check, because under #3848 they will be valid.

Is there some reference for what design you're trying to implement?

The main purpose of this PR of this is to support declarations of members of parameterized types:

class C(T:! type) {
  fn F();
}
fn C(T:! type).F() {}

The most recent / specific proposal on that is #3763. I think this syntax has been part of the intended design for generics for a while, but we don't seem to have any examples of this in /docs/design yet :(

jonmeow · 2024-05-28T21:01:02Z

toolchain/parse/node_kind.def

+//   _repeated_: NameQualifier
+//   _external_: IdentifierName
+//   _optional_ _external_: ImplicitParamList
+//   _optional_ _external_: TuplePattern
+// _declaration name_


This comment is unclear to me: it offers a tree structure that, as far as I can tell, does not exist. Had you considered a non-tree-like representation?

How would you like this to be presented?

Alternatively: how would you feel about removing all of these structure comments now that we have typed_nodes.h that describes the structure?

Removing all these comments seems fair.

jonmeow · 2024-05-28T21:04:54Z

toolchain/parse/state.def

+//  ???
+// ^


What happened here?

This is trying to say: if we're in the DeclNameAndParams state, and the first token isn't a name, that's an error and we're done. (We don't check there's a name next before pushing the state.)

Maybe this comment could be made clearer?

toolchain/parse/state.def

jonmeow · 2024-05-28T21:05:42Z

toolchain/parse/state.def

+// name [ ... ] ( ... ) .
+//                      ^
+//   1. DeclNameAndParams
+//
+// name [ ... ] ( ... ) ...
+//                     ^
+//   (state done)


I'm confused, how are these two inputs different? The . versus ... looks like a typo.

The first rule says: if the next token is a ., then consume it (because it's underlined), and continue in state DeclNameAndParams.

The second rule says: if the next token is anything else (per the comment at the start of the file: "... indicates a sequence of tokens handled by some other state"), then consume nothing and we're done.

Okay, but maybe the comment could be clearer? Maybe this is a problem with using ... without any extra annotation, it makes . look like a typo.

toolchain/parse/testdata/generics/impl/fail_out_of_line_member.carbon

toolchain/parse/handle_decl_name_and_params.cpp

toolchain/check/handle_function.cpp

jonmeow · 2024-05-28T21:30:00Z

I'm not familiar with #3848, and I'm having trouble loading it because it has merge errors.

I believe the PR is only intending to add its p3848.md file and the rest of the changes listed there are spurious.

Thanks -- to be sure, I really meant, it wasn't loading for me to try and find the proposal file. The link helped.

jonmeow

LGTM, although with some hesitance around bracketing.

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

zygoloid requested a review from jonmeow May 24, 2024 23:38

github-actions bot requested a review from geoffromer May 24, 2024 23:38

github-actions bot added the toolchain label May 24, 2024

zygoloid added 3 commits May 24, 2024 23:43

Underline parameters when rejecting them.

b1ae4eb

Cleanups.

5058d05

Fix unit test.

30ec4bc

jonmeow reviewed May 28, 2024

View reviewed changes

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

Fix typo.

b30b41b

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

jonmeow reviewed May 28, 2024

View reviewed changes

jonmeow approved these changes May 28, 2024

View reviewed changes

zygoloid and others added 2 commits May 28, 2024 15:36

Fix numbering.

4abfc85

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

Review comment.

c99f62a

zygoloid added this pull request to the merge queue May 28, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 29, 2024

zygoloid added this pull request to the merge queue May 29, 2024

Merged via the queue into carbon-language:trunk with commit 28170c7 May 29, 2024
7 checks passed

zygoloid deleted the toolchain-generic-members branch May 29, 2024 00:59

jonmeow mentioned this pull request Jan 9, 2025

Track params in the parser #4777

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse parameters in name qualifiers. #3988

Parse parameters in name qualifiers. #3988

zygoloid commented May 24, 2024

jonmeow left a comment

zygoloid commented May 28, 2024 •

edited

Loading

jonmeow May 28, 2024

zygoloid May 28, 2024

jonmeow May 29, 2024

jonmeow May 28, 2024

zygoloid May 28, 2024

jonmeow May 29, 2024

jonmeow May 28, 2024

zygoloid May 28, 2024

jonmeow May 29, 2024

jonmeow commented May 28, 2024

jonmeow left a comment

Parse parameters in name qualifiers. #3988

Parse parameters in name qualifiers. #3988

Conversation

zygoloid commented May 24, 2024

jonmeow left a comment

Choose a reason for hiding this comment

zygoloid commented May 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonmeow commented May 28, 2024

jonmeow left a comment

Choose a reason for hiding this comment

zygoloid commented May 28, 2024 •

edited

Loading