Clairify `ast::PatKind::Struct` presese of `..` by using an enum instead of a bool #119231

aDotInTheVoid · 2023-12-22T23:30:51Z

The bool is mainly used for when a .. is present, but it is also set on recovery to avoid errors. The doc comment not describes both of these cases.

See

rust/compiler/rustc_parse/src/parser/pat.rs

Lines 890 to 897 in cee794e

    
           let (fields, etc) = self.parse_pat_fields().unwrap_or_else(|mut e| { 
        
               e.span_label(path.span, "while parsing the fields for this pattern"); 
        
               e.emit(); 
        
               self.recover_stmt(); 
        
               (ThinVec::new(), true) 
        
           }); 
        
           self.bump(); 
        
           Ok(PatKind::Struct(qself, path, fields, etc))

for the only place this is constructed.

r? @compiler-errors

compiler-errors

This looks good to me, but I wonder if we could make it better

compiler-errors · 2023-12-22T23:34:57Z

compiler/rustc_ast/src/ast.rs

-    /// The `bool` is `true` in the presence of a `..`.
-    Struct(Option<P<QSelf>>, Path, ThinVec<PatField>, /* recovered */ bool),
+    /// The `bool` is `true` in the presence of a `..` (or when recovered).
+    Struct(Option<P<QSelf>>, Path, ThinVec<PatField>, bool),


Please either convert this into:

Struct { qself: Option<P<QSelf>>, path: Path, fields: ThinVec<PatField>, // Whether `..` is meant to capture the rest of the tuple fields rest: bool, }

Or alternatively, change the bool into an enum like StructRest is used in StructExpr:

enum PatFieldsRest { Rest, None, }

Do you think you could do either of these? I have no preference which, but I think we could do better 😸

I think the latter is preferable, as it also clarifies it in the return type in the parser.

Should I make the same change in HIR? Or can I leave that as a potential followup commit?

rustbot · 2023-12-23T00:10:53Z

Could not assign reviewer from: compiler-errors.
User(s) compiler-errors are either the PR author, already assigned, or on vacation, and there are no other candidates.
Use r? to specify someone else to assign.

aDotInTheVoid · 2023-12-23T00:12:11Z

Pushed a version using the enum approach, only in the AST.

compiler-errors

r=me when it's green

compiler-errors · 2023-12-23T00:12:48Z

compiler/rustc_ast_lowering/src/pat.rs

@@ -82,7 +82,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
                                span: self.lower_span(f.span),
                            }
                        }));
-                        break hir::PatKind::Struct(qpath, fs, *etc);
+                        break hir::PatKind::Struct(qpath, fs, *etc == ast::PatFieldsRest::Rest);


a follow-up perhaps

rustbot · 2023-12-23T00:43:22Z

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

aDotInTheVoid · 2023-12-23T00:46:50Z

Pushed a fix to tools. Should I add .is_rest() and .is_none() methods to PatFieldsRest, to avoid needing a load of ugly == methods everywhere?

compiler-errors · 2023-12-23T00:51:15Z

src/tools/rustfmt/src/patterns.rs

@@ -324,7 +327,7 @@ fn rewrite_struct_pat(

    let has_trailing_comma = fmt.needs_trailing_separator();

-    if ellipsis {
+    if ellipsis == ast::PatFieldsRest::None {


This is wrong

compiler-errors · 2023-12-23T00:52:19Z

Should I add .is_rest() and .is_none() methods to PatFieldsRest, to avoid needing a load of ugly == methods everywhere?

Nah I think it's fine

compiler-errors · 2023-12-23T01:30:31Z

thanks!

@bors r+

bors · 2023-12-23T01:30:34Z

📌 Commit 03bed27 has been approved by compiler-errors

It is now in the queue for this repository.

aDotInTheVoid · 2023-12-23T01:37:13Z

sorry about making you review changes that i should have figured were broken here. i'll try to be less careless and make sure everything builds before opening next time

src/tools/rustfmt/src/patterns.rs

compiler-errors · 2023-12-23T02:40:20Z

Let's do that I guess then

@bors r-

@bors rollup

r=me after minimizing the rustfmt change

See https://github.com/rust-lang/rust/blob/cee794ee98d49b45a55ba225680d98e0c4672736/compiler/rustc_parse/src/parser/pat.rs#L890-L897 for the only place this is constructed.

compiler-errors · 2023-12-23T02:56:12Z

@bors r+

bors · 2023-12-23T02:56:14Z

📌 Commit 1349d86 has been approved by compiler-errors

It is now in the queue for this repository.

…docs, r=compiler-errors Clairify `ast::PatKind::Struct` presese of `..` by using an enum instead of a bool The bool is mainly used for when a `..` is present, but it is also set on recovery to avoid errors. The doc comment not describes both of these cases. See https://github.com/rust-lang/rust/blob/cee794ee98d49b45a55ba225680d98e0c4672736/compiler/rustc_parse/src/parser/pat.rs#L890-L897 for the only place this is constructed. r? `@compiler-errors`

…iaskrgr Rollup of 2 pull requests Successful merges: - rust-lang#119072 (Clean up `check_consts` and misc fixes) - rust-lang#119231 (Clairify `ast::PatKind::Struct` presese of `..` by using an enum instead of a bool) r? `@ghost` `@rustbot` modify labels: rollup

…iaskrgr Rollup of 5 pull requests Successful merges: - rust-lang#119231 (Clairify `ast::PatKind::Struct` presese of `..` by using an enum instead of a bool) - rust-lang#119232 (Fix doc typos) - rust-lang#119245 (Improve documentation for using warning blocks in documentation) - rust-lang#119248 (remove dead inferred outlives testing code) - rust-lang#119249 (Add spastorino to users_on_vacation) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#119231 - aDotInTheVoid:PatKind-struct-bool-docs, r=compiler-errors Clairify `ast::PatKind::Struct` presese of `..` by using an enum instead of a bool The bool is mainly used for when a `..` is present, but it is also set on recovery to avoid errors. The doc comment not describes both of these cases. See https://github.com/rust-lang/rust/blob/cee794ee98d49b45a55ba225680d98e0c4672736/compiler/rustc_parse/src/parser/pat.rs#L890-L897 for the only place this is constructed. r? ``@compiler-errors``

…docs, r=compiler-errors Clairify `ast::PatKind::Struct` presese of `..` by using an enum instead of a bool The bool is mainly used for when a `..` is present, but it is also set on recovery to avoid errors. The doc comment not describes both of these cases. See https://github.com/rust-lang/rust/blob/cee794ee98d49b45a55ba225680d98e0c4672736/compiler/rustc_parse/src/parser/pat.rs#L890-L897 for the only place this is constructed. r? ``@compiler-errors``

rustbot assigned compiler-errors Dec 22, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 22, 2023

compiler-errors approved these changes Dec 22, 2023

View reviewed changes

rust-cloud-vms bot force-pushed the PatKind-struct-bool-docs branch from 5627b0e to 2b620be Compare December 23, 2023 00:10

aDotInTheVoid changed the title ~~Clairify ast::PatKind::Struct docs.~~ Clairify ast::PatKind::Struct presese of .. by using an enum instead of a bool Dec 23, 2023

compiler-errors approved these changes Dec 23, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

rust-cloud-vms bot force-pushed the PatKind-struct-bool-docs branch from 2b620be to af149df Compare December 23, 2023 00:43

compiler-errors reviewed Dec 23, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

rust-cloud-vms bot force-pushed the PatKind-struct-bool-docs branch from af149df to 03bed27 Compare December 23, 2023 01:03

compiler-errors approved these changes Dec 23, 2023

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 23, 2023

ytmimi reviewed Dec 23, 2023

View reviewed changes

src/tools/rustfmt/src/patterns.rs Show resolved Hide resolved

This comment was marked as off-topic.

Sign in to view

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 23, 2023

bool->enum for ast::PatKind::Struct presence of ..

1349d86

See https://github.com/rust-lang/rust/blob/cee794ee98d49b45a55ba225680d98e0c4672736/compiler/rustc_parse/src/parser/pat.rs#L890-L897 for the only place this is constructed.

rust-cloud-vms bot force-pushed the PatKind-struct-bool-docs branch from 03bed27 to 1349d86 Compare December 23, 2023 02:50

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 23, 2023

matthiaskrgr mentioned this pull request Dec 23, 2023

Rollup of 2 pull requests #119243

Closed

matthiaskrgr mentioned this pull request Dec 23, 2023

Rollup of 5 pull requests #119256

Merged

bors merged commit bdc4480 into rust-lang:master Dec 23, 2023

rustbot added this to the 1.77.0 milestone Dec 23, 2023

xFrednet mentioned this pull request Dec 23, 2023

Chore: Bump nightly -> 2023-12-28 rust-marker/marker#315

Open

	let (fields, etc) = self.parse_pat_fields().unwrap_or_else(\|mut e\| {
	e.span_label(path.span, "while parsing the fields for this pattern");
	e.emit();
	self.recover_stmt();
	(ThinVec::new(), true)
	});
	self.bump();
	Ok(PatKind::Struct(qself, path, fields, etc))

Clairify ast::PatKind::Struct presese of .. by using an enum instead of a bool #119231

Clairify ast::PatKind::Struct presese of .. by using an enum instead of a bool #119231

Uh oh!

Conversation

aDotInTheVoid commented Dec 22, 2023

Uh oh!

compiler-errors left a comment

Choose a reason for hiding this comment

Uh oh!

compiler-errors Dec 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aDotInTheVoid Dec 22, 2023

Choose a reason for hiding this comment

Uh oh!

aDotInTheVoid Dec 23, 2023

Choose a reason for hiding this comment

Uh oh!

rustbot commented Dec 23, 2023

Uh oh!

aDotInTheVoid commented Dec 23, 2023

Uh oh!

compiler-errors left a comment

Choose a reason for hiding this comment

Uh oh!

compiler-errors Dec 23, 2023

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

rustbot commented Dec 23, 2023

Uh oh!

aDotInTheVoid commented Dec 23, 2023

Uh oh!

compiler-errors Dec 23, 2023

Choose a reason for hiding this comment

Uh oh!

compiler-errors commented Dec 23, 2023

Uh oh!

This comment has been minimized.

compiler-errors commented Dec 23, 2023

Uh oh!

bors commented Dec 23, 2023

Uh oh!

aDotInTheVoid commented Dec 23, 2023

Uh oh!

Uh oh!

This comment was marked as off-topic.

compiler-errors commented Dec 23, 2023

Uh oh!

compiler-errors commented Dec 23, 2023

Uh oh!

bors commented Dec 23, 2023

Uh oh!

Uh oh!

Clairify `ast::PatKind::Struct` presese of `..` by using an enum instead of a bool #119231

Clairify `ast::PatKind::Struct` presese of `..` by using an enum instead of a bool #119231

compiler-errors Dec 22, 2023 •

edited

Loading