Skip to content
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.

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

Suggest removing leading left angle brackets. #57852

Merged
merged 2 commits into from
Jan 26, 2019

Conversation

davidtwco
Copy link
Member

Fixes #57819.

This PR adds errors and accompanying suggestions as below:

bar::<<<<<T as Foo>::Output>();
     ^^^ help: remove extra angle brackets

r? @estebank

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 23, 2019
@davidtwco
Copy link
Member Author

I'm not sure how this interacts with #57817 if there are both extra trailing and leading angle brackets.

@estebank
Copy link
Contributor

estebank commented Jan 23, 2019

A cursory look at the code tells me that's fine, but I wonder how often this case happens as opposed to missing trailing brackets. What's the output of this PR for the following?

println!("{:?}", vec![1, 2, 3].into_iter().collect::<Vec<usize>());
println!("{:?}", vec![1, 2, 3].into_iter().collect::<<<Vec<usize>>());
println!("{:?}", vec![1, 2, 3].into_iter().collect::<<<Vec<usize>>>());
println!("{:?}", vec![1, 2, 3].into_iter().collect::<<Vec<usize>>>());
bar::<<T as Foo>::Output();

This commit adds errors and accompanying suggestions as below:

```
bar::<<<<<T as Foo>::Output>();
     ^^^ help: remove extra angle brackets
```
@davidtwco
Copy link
Member Author

A cursory look at the code tells me that's fine, but I wonder how often this case happens as opposed to missing trailing brackets. What's the output of this PR for the following?

I've put the output of this PR as well as the output of this PR and #57817 at the same time in this gist. I think this behaves appropriately.

Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

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

I am slightly worried about the snapshot usage here (which again, it might be fine), but I'm thinking that we might be able to use unmatched_angle_bracket_count in expected_one_of...

src/libsyntax/parse/parser.rs Outdated Show resolved Hide resolved
src/libsyntax/parse/parser.rs Outdated Show resolved Hide resolved
@estebank
Copy link
Contributor

@bors try

@bors
Copy link
Contributor

bors commented Jan 23, 2019

⌛ Trying commit 22f794b with merge 100d423c5c506b60b6ab591dfea5515f65a7fb74...

@estebank
Copy link
Contributor

By the way, I love the level of recovery.

With the introduction of unmatched_angle_bracket_count we can make expected_one_of much smarter. If > is in the list, like in the following example, we could actually give a suggestion and recover:

error: expected one of `,`, `->`, `::`, or `>`, found `;`
 --> test.rs:6:31
  |
6 |     bar::<<T as Foo>::Output();
  |                               ^
  |                               |
  |                               expected one of `,`, `->`, `::`, or `>` here
  |                               help: likely there's a missing `>` here

This commit implements a suggestion from @estebank that optimizes the
use of snapshots.

Instead of creating a snapshot for each recursion in `parse_path_segment`
and then replacing `self` with them until the first invocation where if
leading angle brackets are detected, `self` is not replaced and instead the
snapshot is used to inform how parsing should continue.

Now, a snapshot is created in the first invocation that acts as a backup
of the parser state before any generic arguments are parsed (and
therefore, before recursion starts). This backup replaces `self` if after
all parsing of generic arguments has concluded we can determine that
there are leading angle brackets. Parsing can then proceed from the
backup state making use of the now known number of unmatched leading
angle brackets to recover.
@bors
Copy link
Contributor

bors commented Jan 23, 2019

☀️ Test successful - checks-travis
State: approved= try=True

@estebank
Copy link
Contributor

@bors try

@bors
Copy link
Contributor

bors commented Jan 23, 2019

⌛ Trying commit 8ab12f6 with merge de98dc84921c98aec5fe83cceb4cda42ba22e555...

@estebank
Copy link
Contributor

@rust-timer build 100d423c5c506b60b6ab591dfea5515f65a7fb74

@bors
Copy link
Contributor

bors commented Jan 24, 2019

☀️ Test successful - checks-travis
State: approved= try=True

@estebank
Copy link
Contributor

@rust-timer build de98dc84921c98aec5fe83cceb4cda42ba22e555

@rust-timer
Copy link
Collaborator

Insufficient permissions to issue commands to rust-timer.

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Jan 24, 2019

@rust-timer build de98dc84921c98aec5fe83cceb4cda42ba22e555

I've also added you to the list

@rust-timer
Copy link
Collaborator

Success: Queued de98dc84921c98aec5fe83cceb4cda42ba22e555 with parent 19f8958, comparison URL.

1 similar comment
@rust-timer
Copy link
Collaborator

Success: Queued de98dc84921c98aec5fe83cceb4cda42ba22e555 with parent 19f8958, comparison URL.

@estebank
Copy link
Contributor

@rust-lang/compiler I'm looking at the perf results and it looks reasonable to me for the most part, but there are some 5% worst case swings in cycles:u and cpu:clock for two of the tests. I'm tempted at approving unless anyone has any objections. Memory usage had the biggest hit (9% for webrender-debug for a clean compile).

@estebank estebank added the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Jan 24, 2019
@nikomatsakis
Copy link
Contributor

My suspicion, @estebank, is that these results are mostly noise. This is my reasoning, let me know if you see a flaw:

  • This code is adding overhead to the parser.
  • Presently, the parser runs in ALL scenarios.
  • However, looking at the comparison link, I see a few random slowdowns that only affect one scenario (example below). I would expect some effect on all scenarios.

For example, consider these results:

unicode_normalization-check avg: 0.6% min: -0.3% max: 1.7%
clean-check 39,380,205,064.00 40,057,925,965.00 1.7%
nll-check 40,821,023,751.00 40,814,681,209.00 -0.0%
baseline incremental-check 47,085,578,219.00 47,768,957,881.00 1.5%
clean incremental-check 1,360,091,673.00 1,356,348,397.00 -0.3%
patched incremental: println-check 41,251,067,880.00 41,249,853,473.00 -0.0%

Here, I would think that if the parser got slower, and it was affecting the "baseline incremental" run (which means the first incremental run, with no prior state), then it ought to affect "clean incremental" even more so -- as clean incremental does the same amount of parsing, but does a lot less other work.

So the main question is whether I am correct that the overhead here would be focused on the parser, I guess. If so, I think it's fine.

@estebank
Copy link
Contributor

I agree that this is only parser overhead. The only regression that seemed relevant was memory usage for webrender-debug for a clean compile of 9% , but even that seems a bit high. Regardless, the parser is a small portion of where rustc spends time, so I feel comfortable merging this. There are also a couple of small tweaks I want to build on top of this.

@bors r+

@bors
Copy link
Contributor

bors commented Jan 25, 2019

📌 Commit 8ab12f6 has been approved by estebank

@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. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jan 25, 2019
Centril added a commit to Centril/rust that referenced this pull request Jan 26, 2019
Suggest removing leading left angle brackets.

Fixes rust-lang#57819.

This PR adds errors and accompanying suggestions as below:

```
bar::<<<<<T as Foo>::Output>();
     ^^^ help: remove extra angle brackets
```

r? @estebank
Centril added a commit to Centril/rust that referenced this pull request Jan 26, 2019
Suggest removing leading left angle brackets.

Fixes rust-lang#57819.

This PR adds errors and accompanying suggestions as below:

```
bar::<<<<<T as Foo>::Output>();
     ^^^ help: remove extra angle brackets
```

r? @estebank
@bors
Copy link
Contributor

bors commented Jan 26, 2019

⌛ Testing commit 8ab12f6 with merge 46a43dc...

bors added a commit that referenced this pull request Jan 26, 2019
Suggest removing leading left angle brackets.

Fixes #57819.

This PR adds errors and accompanying suggestions as below:

```
bar::<<<<<T as Foo>::Output>();
     ^^^ help: remove extra angle brackets
```

r? @estebank
@bors
Copy link
Contributor

bors commented Jan 26, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: estebank
Pushing 46a43dc to master...

@bors bors merged commit 8ab12f6 into rust-lang:master Jan 26, 2019
@davidtwco davidtwco deleted the issue-57819 branch January 26, 2019 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants