Add `require` node(s) to the parse tree #4210

gysddn · 2024-08-09T04:00:28Z

Added new nodes to the parse tree to handle require declarations

Require is a new bracketed parse node with the following structure:

Require
-Impls (New node for expr impls expr statements)
- AnyExpr
- AnyExpr
-RequireIntroducer (Bracket node)

Two test cases added for the usage in the interfaces.

Added new nodes to the parse tree to handle require declarations `Require` is a new bracketed parse node with the following structure: Require -Impls (New node for `expr impls expr` statement) - AnyExpr - AnyExpr -RequireIntroducer (Bracket node) Two test cases added for the usage in the interfaces.

jonmeow

@josh11b I'm trying to figure out how to review this. Looking at the generics design, I see examples of its use, but I'm having trouble finding what the full syntax is. Is the syntax in the design somewhere that I'm missing? Some of the examples include where, does this overlap with the where parsing you've been looking at?

@gysddn FYI, the items on Toolchain tasks are things that are closer to our development focus. I'm going to need some time to figure out what the syntax is intended to be before I can really review, but I've left some comments regarding testing since those are more general comments not specific to this change. It's important to be testing both success cases and failure cases.

toolchain/parse/testdata/generics/interface/require_name.carbon

jonmeow · 2024-08-09T16:42:15Z

toolchain/parse/testdata/generics/interface/require_name.carbon

+interface Bar {
+  require i32 impls Foo;
+}


Can you add tests of incorrect syntax? There are a lot of possible typos here, and we should make sure error handling is working.

Just as a few examples:

// Missing components. require; require impls Foo; require i32; require i32 impls; // Extra expressions. require i32 i32 impls Foo; require i32 impls Foo Foo;

Plus, what happens when there's a missing semicolon?

It looks like you've added most of these, but is there a test case with a missing semicolon?

e.g.:

interface Foo { require i32 impls Bar }

I expect this to work fine, but having tests is important to both catch unexpected results and prevent regressions.

toolchain/parse/testdata/generics/interface/require_name.carbon

josh11b · 2024-08-09T21:11:03Z

@josh11b I'm trying to figure out how to review this. Looking at the generics design, I see examples of its use, but I'm having trouble finding what the full syntax is. Is the syntax in the design somewhere that I'm missing? Some of the examples include where, does this overlap with the where parsing you've been looking at?

The argument to require is supposed to match the argument to where, which I recently was working on -- see #4075 . The arguments are described in https://github.com/carbon-language/carbon-lang/blob/trunk/docs/design/generics/details.md#where-constraints -- though the description is spread out across that whole section, without a short summary of just the syntax.

One thing to note, though, that in discussions we've decided that the argument to a where clause is not an expression, and so it has its own distinct interpretation of symbols like == that appear in ordinary expressions.

jonmeow · 2024-08-15T15:55:15Z

The argument to require is supposed to match the argument to where, which I recently was working on -- see #4075 . The arguments are described in https://github.com/carbon-language/carbon-lang/blob/trunk/docs/design/generics/details.md#where-constraints -- though the description is spread out across that whole section, without a short summary of just the syntax.

So to be sure I understand correctly, require actually has a single argument, and that argument is the same as an argument to where? i.e., the impls just happens to be in all the examples?

(if that understanding is correct, probably this change should be reverted since josh11b is already working on where syntax, and this change isn't going to provide the right support)

josh11b · 2024-08-15T16:47:35Z

The argument to require is supposed to match the argument to where, which I recently was working on -- see #4075 . The arguments are described in https://github.com/carbon-language/carbon-lang/blob/trunk/docs/design/generics/details.md#where-constraints -- though the description is spread out across that whole section, without a short summary of just the syntax.

So to be sure I understand correctly, require actually has a single argument, and that argument is the same as an argument to where? i.e., the impls just happens to be in all the examples?

(if that understanding is correct, probably this change should be reverted since josh11b is already working on where syntax, and this change isn't going to provide the right support)

https://github.com/carbon-language/carbon-lang/blob/trunk/proposals/p2760.md#proposal

For now, only the impls forms of where clauses are permitted after require.

Eventually my expectation is that we would accept any kind of where clause after require when parsing, and then add restrictions in the check stage; but for now it is totally fine to only accept impls after require in parse.

gysddn · 2024-08-16T00:22:58Z

So to be sure I understand correctly, require actually has a single argument, and that argument is the same as an argument to where? i.e., the impls just happens to be in all the examples?

Yeah, I didn't really realize this while making these changes.
If the syntax will be provided by @josh11b changes, then I can update this PR or just make another one.

jonmeow · 2024-08-16T15:21:00Z

To be sure we're all on the same page:

Josh is fine with the basic syntax support being added.
- That is, require <expr> impls <expr>;.
Per my comments, edge cases and incorrect syntax must be tested.
- The parse tree must be balanced even for parse trees that contain error nodes.

toolchain/parse/typed_nodes.h

- Merge tests into one file - Add more cases to cover incorrect syntax

- Make `Require` node to be a fixed children size type (.child_count = 2) instead of bracketed.

jonmeow

Thanks! I think the behavior looks good.

I'm suggesting changes to the state.def comments in order to try and match the file's style better. If I'm misreading something there, please let me know.

jonmeow · 2024-08-20T16:22:02Z

toolchain/parse/handle_require.cpp

+  auto state = context.PopState();
+
+  if (auto impls = context.ConsumeIf(Lex::TokenKind::Impls)) {
+    // context.AddNode(NodeKind::ImplsTypeAs, *impls, state.has_error);


Suggested change

// context.AddNode(NodeKind::ImplsTypeAs, *impls, state.has_error);

Was this leftover from work? Should it be removed?

Oh yeah, sorry about that 😅

jonmeow · 2024-08-20T16:31:46Z

toolchain/parse/state.def

+// require ...
+// ^~~~~~~
+//   1. Require
+//   2. DeclScopeLoop
+//


Looking at surrounding entries, we're adding them lexically (alias, base, choice, etc). Can you please match that style, placing after package?

jonmeow · 2024-08-20T16:33:40Z

toolchain/parse/state.def

+//
+CARBON_PARSE_STATE(Impls)
+
+// Finishes impls processing


Suggested change

// Finishes impls processing

// Finishes `impls` processing.

//

As above. Also, fix punctuation.

jonmeow · 2024-08-20T16:34:56Z

toolchain/parse/state.def

+CARBON_PARSE_STATE(Require)
+
+// Handles the processing of `impls` in a `require` declaration after the type
+// expression.


Suggested change

// expression.

// expression.

//

Similar to other comments, let's put a blank line separating comment from example.

jonmeow · 2024-08-20T16:35:33Z

toolchain/parse/state.def

+
+CARBON_PARSE_STATE(ImplsFinish)
+
+// Finishes `require` processing


Suggested change

// Finishes `require` processing

// Finishes `require` processing.

//

As above.

jonmeow · 2024-08-20T17:13:19Z

toolchain/parse/state.def

+//   1. Expr
+//   2. Impls
+//   3. RequireFinish
+//


Suggested change

//

We typically don't put blank lines at the end of a comment block.

jonmeow · 2024-08-20T17:15:20Z

toolchain/parse/state.def

+// impls ...
+// ^~~~~
+//
+//   1. Expr
+//   2. ImplsFinish
+//


The syntax you're documenting here is for the error case, but you have it for the expression case. Let's document both paths.

Also note this suggestion is fixing extra blank lines.

Suggested change

// impls ...

// ^~~~~

//

// 1. Expr

// 2. ImplsFinish

//

// expr impls ...

// ^~~~~

// 1. Expr

// 2. ImplsFinish

//

// expr ???

// ^

// (state done)

jonmeow · 2024-08-20T17:23:58Z

toolchain/parse/state.def

+// impls type_expression ...
+//                      ^


Suggested change

// impls type_expression ...

// ^

// expr impls expr

// ^

Let's include the expr before impls here.

jonmeow · 2024-08-20T17:28:51Z

toolchain/parse/state.def

+//                      ^
+//   (state done)
+//
+


Suggested change

jonmeow · 2024-08-20T17:29:43Z

toolchain/parse/state.def

+// impls type_expression ...
+//                      ^
+//   (state done)
+//


Suggested change

// impls type_expression ...

// ^

// (state done)

//

// expr impls expr

// ^

// (state done)

Similar to impls, I think the syntax example can be more specific. When there is no following syntax handled, we omit the trailing "...". Also, since parsing only checks for an expression, let's only say "expr".

Updated the comments

github-actions bot added the toolchain label Aug 9, 2024

github-actions bot requested a review from jonmeow August 9, 2024 04:00

jonmeow reviewed Aug 9, 2024

View reviewed changes

jonmeow reviewed Aug 16, 2024

View reviewed changes

toolchain/parse/typed_nodes.h Outdated Show resolved Hide resolved

gysddn added 3 commits August 16, 2024 19:56

Update tests to split-file format

380e845

- Merge tests into one file - Add more cases to cover incorrect syntax

Update Require node to be fixed size

598cc1c

- Make `Require` node to be a fixed children size type (.child_count = 2) instead of bracketed.

Merge branch 'trunk' into impl_req

b52273c

jonmeow reviewed Aug 20, 2024

View reviewed changes

gysddn added 2 commits September 7, 2024 06:15

remove unused commented line

0fa5656

fix states.def comment problems

02f883d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `require` node(s) to the parse tree #4210

Add `require` node(s) to the parse tree #4210

gysddn commented Aug 9, 2024

jonmeow left a comment •

edited

Loading

jonmeow Aug 9, 2024

jonmeow Aug 20, 2024

josh11b commented Aug 9, 2024 •

edited

Loading

jonmeow commented Aug 15, 2024 •

edited

Loading

josh11b commented Aug 15, 2024

gysddn commented Aug 16, 2024

jonmeow commented Aug 16, 2024 •

edited

Loading

jonmeow left a comment

jonmeow Aug 20, 2024

gysddn Sep 9, 2024

jonmeow Aug 20, 2024

jonmeow Aug 20, 2024

jonmeow Aug 20, 2024

jonmeow Aug 20, 2024

jonmeow Aug 20, 2024

jonmeow Aug 20, 2024

jonmeow Aug 20, 2024

jonmeow Aug 20, 2024

jonmeow Aug 20, 2024

gysddn Sep 9, 2024

	// Finishes impls processing
	// Finishes `impls` processing.
	//


		CARBON_PARSE_STATE(ImplsFinish)

		// Finishes `require` processing

	// Finishes `require` processing
	// Finishes `require` processing.
	//

Add require node(s) to the parse tree #4210

Are you sure you want to change the base?

Add require node(s) to the parse tree #4210

Conversation

gysddn commented Aug 9, 2024

jonmeow left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josh11b commented Aug 9, 2024 • edited Loading

jonmeow commented Aug 15, 2024 • edited Loading

josh11b commented Aug 15, 2024

gysddn commented Aug 16, 2024

jonmeow commented Aug 16, 2024 • edited Loading

jonmeow left a 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

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

Add `require` node(s) to the parse tree #4210

Add `require` node(s) to the parse tree #4210

jonmeow left a comment •

edited

Loading

josh11b commented Aug 9, 2024 •

edited

Loading

jonmeow commented Aug 15, 2024 •

edited

Loading

jonmeow commented Aug 16, 2024 •

edited

Loading