Skip to content

Less NoDelim #96421

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Apr 28, 2022
Merged

Less NoDelim #96421

merged 5 commits into from
Apr 28, 2022

Conversation

@nnethercote
Copy link
Contributor

Currently there are several places where NoDelim (which really means "implicit delimiter" or "invisible delimiter") is used to mean "no delimiter". The name NoDelim is a bit misleading, and may be a cause.

This PR changes these places, e.g. by changing a DelimToken to Option<DelimToken> and then using None to mean "no delimiter". As a result, the only place where NoDelim values are now produced is within:

  • Delimiter::to_internal(), when converting from Delimiter::None.
  • FlattenNonterminals::process_token(), when converting TokenKind::Interpolated.

r? @petrochenkov

@rust-highfive
Copy link
Contributor

Some changes occurred in src/tools/rustfmt.

cc @rust-lang/rustfmt

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 26, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 26, 2022
@nnethercote
Copy link
Contributor Author

@petrochenkov: see what you think of this. I was trying to understand NoDelim better, and clearing this up improved my understanding, by making it easier to see where the "real" NoDelim uses are.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
petrochenkov
petrochenkov reviewed Apr 26, 2022
View reviewed changes
compiler/rustc_parse/src/parser/attr_wrapper.rs Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still necessary?
There's no longer a DelimToken::NoDelim at the top level.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's also a similar // HACK comment lower in the same function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "mismatched None delimiter at the top level" here refers to token, which comes from the iterator, not the frame. Indeed, the condition looks for a non-NoDelim in the frame. So this change preserves the existing behaviour, as I understand it.

More generally: there are three HACK comments in this function, and a comment at the top of the function referring to #67062. The HACK comments are on three if statements. I tried changing the bodies of those three if statements to panic!() and the test suite passed. I then tried removing all three if statements and the test suite again passed.

These if statements were added in #82608, with an explanation here. I don't know if (a) they never did anything useful, (b) they do something useful but we have no test coverage for them, or (c) something has changed in the meantime that means they are no longer necessary.

I suggest we leave this change as is, and consider removing these hacks in a separate PR. That way if there are any regressions they'll be easier to deal with.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have filed #96543 to remove the hacks.

@petrochenkov petrochenkov 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-review Status: Awaiting review from the assignee but also interested parties. labels Apr 26, 2022
@petrochenkov petrochenkov mentioned this pull request Apr 26, 2022
Merged
@nnethercote
Avoid producing NoDelim values in TokenCursorFrame.
f0bbc78
@nnethercote
Avoid producing NoDelim values in MacArgs::delim().
6b367a0
@nnethercote
Avoid producing NoDelim values in Frame.
a8e862c 
The code currently ignores the actual delimiter on the RHS and fakes up
a `NoDelim`/`DelimSpan::dummy()` one. This commit changes it to use the
actual delimiter.

The commit also reorders the fields for the `Delimited` variant to match
the `Sequence` variant.
@nnethercote
Avoid producing NoDelim values in FrameData.
9665da3
@nnethercote
Make explicit an unreachable NoDelim case in rustfmt.
86f0117
@nnethercote
Copy link
Contributor Author

I have addresses the comments, except for those explained above.

@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 27, 2022
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Apr 27, 2022

📌 Commit 86f0117 has been approved by petrochenkov

@bors 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 Apr 27, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 27, 2022
@Dylan-DPC
Rollup merge of rust-lang#96421 - nnethercote:less-NoDelim, r=petroch…
0f79041 
…enkov

Less `NoDelim`

Currently there are several places where `NoDelim` (which really means "implicit delimiter" or "invisible delimiter") is used to mean "no delimiter". The name `NoDelim` is a bit misleading, and may be a cause.

This PR changes these places, e.g. by changing a `DelimToken` to `Option<DelimToken>` and then using `None` to mean "no delimiter". As a result, the *only* place where `NoDelim` values are now produced is within:
- `Delimiter::to_internal()`, when converting from `Delimiter::None`.
- `FlattenNonterminals::process_token()`, when converting `TokenKind::Interpolated`.

r? `@petrochenkov`
@Dylan-DPC Dylan-DPC mentioned this pull request Apr 27, 2022
Closed
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 27, 2022
@Dylan-DPC
Rollup merge of rust-lang#96421 - nnethercote:less-NoDelim, r=petroch…
f485572 
…enkov

Less `NoDelim`

Currently there are several places where `NoDelim` (which really means "implicit delimiter" or "invisible delimiter") is used to mean "no delimiter". The name `NoDelim` is a bit misleading, and may be a cause.

This PR changes these places, e.g. by changing a `DelimToken` to `Option<DelimToken>` and then using `None` to mean "no delimiter". As a result, the *only* place where `NoDelim` values are now produced is within:
- `Delimiter::to_internal()`, when converting from `Delimiter::None`.
- `FlattenNonterminals::process_token()`, when converting `TokenKind::Interpolated`.

r? ``@petrochenkov``
@Dylan-DPC Dylan-DPC mentioned this pull request Apr 27, 2022
Closed
@Dylan-DPC Dylan-DPC mentioned this pull request Apr 28, 2022
Closed
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 28, 2022
@Dylan-DPC
Rollup merge of rust-lang#96421 - nnethercote:less-NoDelim, r=petroch…
8eaac7a 
…enkov

Less `NoDelim`

Currently there are several places where `NoDelim` (which really means "implicit delimiter" or "invisible delimiter") is used to mean "no delimiter". The name `NoDelim` is a bit misleading, and may be a cause.

This PR changes these places, e.g. by changing a `DelimToken` to `Option<DelimToken>` and then using `None` to mean "no delimiter". As a result, the *only* place where `NoDelim` values are now produced is within:
- `Delimiter::to_internal()`, when converting from `Delimiter::None`.
- `FlattenNonterminals::process_token()`, when converting `TokenKind::Interpolated`.

r? ```@petrochenkov```
@Dylan-DPC Dylan-DPC mentioned this pull request Apr 28, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 28, 2022
@bors
Auto merge of rust-lang#96495 - Dylan-DPC:rollup-9lm4tpp, r=Dylan-DPC
81799cd 
Rollup of 7 pull requests

Successful merges:

 - rust-lang#96377 (make `fn() -> _ { .. }` suggestion MachineApplicable)
 - rust-lang#96397 (Make EncodeWide implement FusedIterator)
 - rust-lang#96421 (Less `NoDelim`)
 - rust-lang#96432 (not need `Option` for `dbg_scope`)
 - rust-lang#96466 (Better error messages when collecting into `[T; n]`)
 - rust-lang#96471 (replace let else with `?`)
 - rust-lang#96483 (Add missing `target_feature` to the list of well known cfg names)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 80045d6 into rust-lang:master Apr 28, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 28, 2022
@nnethercote nnethercote deleted the less-NoDelim branch April 28, 2022 05:50
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 28, 2022
@Dylan-DPC
Rollup merge of rust-lang#96433 - petrochenkov:delim, r=nnethercote
d6905ce 
rustc_ast: Harmonize delimiter naming with `proc_macro::Delimiter`

Compiler cannot reuse `proc_macro::Delimiter` directly due to extra impls, but can at least use the same naming.

After this PR the only difference between these two enums is that `proc_macro::Delimiter::None` is turned into `token::Delimiter::Invisible`.
It's my mistake that the invisible delimiter is called `None` on stable, during the stabilization I audited the naming and wrote the docs, but missed the fact that the `None` naming gives a wrong and confusing impression about what this thing is.

cc rust-lang#96421
r? `@nnethercote`
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 28, 2022
@Dylan-DPC
Rollup merge of rust-lang#96433 - petrochenkov:delim, r=nnethercote
0cbf3b2 
rustc_ast: Harmonize delimiter naming with `proc_macro::Delimiter`

Compiler cannot reuse `proc_macro::Delimiter` directly due to extra impls, but can at least use the same naming.

After this PR the only difference between these two enums is that `proc_macro::Delimiter::None` is turned into `token::Delimiter::Invisible`.
It's my mistake that the invisible delimiter is called `None` on stable, during the stabilization I audited the naming and wrote the docs, but missed the fact that the `None` naming gives a wrong and confusing impression about what this thing is.

cc rust-lang#96421
r? ``@nnethercote``
workingjubilee pushed a commit to tcdi/postgrestd that referenced this pull request Sep 15, 2022
@Dylan-DPC
Rollup merge of #96433 - petrochenkov:delim, r=nnethercote
b127237 
rustc_ast: Harmonize delimiter naming with `proc_macro::Delimiter`

Compiler cannot reuse `proc_macro::Delimiter` directly due to extra impls, but can at least use the same naming.

After this PR the only difference between these two enums is that `proc_macro::Delimiter::None` is turned into `token::Delimiter::Invisible`.
It's my mistake that the invisible delimiter is called `None` on stable, during the stabilization I audited the naming and wrote the docs, but missed the fact that the `None` naming gives a wrong and confusing impression about what this thing is.

cc rust-lang/rust#96421
r? ``@nnethercote``
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

Assignees

@petrochenkov petrochenkov

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

1.62.0

Development

Successfully merging this pull request may close these issues.

6 participants

@nnethercote @rust-highfive @rust-log-analyzer @petrochenkov @bors @rustbot