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 enclosing const expression in block #64700

Closed
wants to merge 3 commits into from

Conversation

estebank
Copy link
Contributor

@estebank estebank commented Sep 23, 2019

When encountering foo::<X + 1>(), suggest foo::<{ X + 1 }>(). When encountering foo::<1 + 1>() or foo::<1 + X>(), parse correctly.

Fix #61175.

@rust-highfive
Copy link
Collaborator

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

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

r? @Centril CC @oli-obk

@rust-highfive rust-highfive assigned Centril and unassigned eddyb Sep 23, 2019
@rust-highfive

This comment has been minimized.

src/libsyntax/ast.rs Outdated Show resolved Hide resolved
src/libsyntax/ast.rs Outdated Show resolved Hide resolved
src/libsyntax/ast.rs Outdated Show resolved Hide resolved
src/libsyntax/ast.rs Outdated Show resolved Hide resolved
src/libsyntax/ast.rs Outdated Show resolved Hide resolved
src/libsyntax/parse/parser/path.rs Outdated Show resolved Hide resolved
src/libsyntax/parse/parser/path.rs Outdated Show resolved Hide resolved
src/libsyntax/parse/parser/path.rs Outdated Show resolved Hide resolved
src/libsyntax/ast.rs Outdated Show resolved Hide resolved
src/libsyntax/parse/parser/expr.rs Outdated Show resolved Hide resolved
@@ -186,6 +186,12 @@ impl<'a> Parser<'a> {
self.last_type_ascription = None;
return Ok(lhs);
}
(true, Some(AssocOp::Greater)) if self.restrictions.contains(
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't say I'm following the changes in this particular file and how it is hanging together & will need some help with that. As this is subtle, it would be good if @petrochenkov could also give this a look.

@estebank estebank force-pushed the recover-const branch 2 times, most recently from 06847d7 to 39ae744 Compare September 23, 2019 19:49
@Centril
Copy link
Contributor

Centril commented Sep 23, 2019

r? @petrochenkov for some more reviewing.

@petrochenkov
Copy link
Contributor

  • I'd want the referenced issue fixed eventually, using a proper isolated fix not increasing technical debt.
  • I don't think a bunch of heuristics spread through the reference code polluting it is a proper fix, and I have little confidence in them behaving correctly (e.g. "const arguments that don't require surrunding braces would have a length one token" is not correct now due to -LIT and can't be relied upon in the future.)
  • I don't know what that proper fix would be without trying to implement it myself, but I'm unlikely to try it because I'm on vacation.
  • A steady stream of similar PRs has been merged for a long time, so I think we are too late, the parser has all been infected. It may look fine now, but it's just a matter of time before it turns into undead. This entire city must be purged.

@petrochenkov petrochenkov removed their assignment Sep 25, 2019
@estebank
Copy link
Contributor Author

We can always close this PR and use it as a reference once const generics is more mature.

I do want to make the case that nightly features are "leaky", particularly in the parser because they cause subpar diagnostics with incomplete code, which is why I don't restrict myself to only stable features. Type ascription is a good example of how bad the diagnostics can be when not tackled, and how hard and far reaching the implications of the chosen syntax are.

@petrochenkov
Copy link
Contributor

I think I have a more constructive suggestion today.
Basically, Foo<1 + 1> needs to just work (*), one of the RFC threads has a discussion about that, but the RFC text was never updated to include its results.

For that to work, we need the expression parsing mode treating closing angle brackets specially anyway (like Restrictions::CONST_EXPR_RECOVERY, but not only for recovery.)
So, I think we should start with implementing it carefully (with correct treatment of >>, etc) and make a separate PR.

(*) Because can_begin_expr(token) == true && can_begin_type(token) == false for token == 1.

(I need to go right now, will find the relevant links later.)
r? @petrochenkov

@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 Sep 26, 2019
@petrochenkov
Copy link
Contributor

Here it is - rust-lang/rfcs#1931 (comment), I was searching on the wrong RFC thread.

So, the logic for parsing a generic argument is

if can_begin_expr(token) && !can_begin_type(token) {
    if !is_whitelisted(token) {
        span_err("conservative error");
    }
    parse_expr(Restrictions::CLOSING_ANGLE_BRACKET)
} else {
    parse_type()
}

where is_whitelisted is {, -, LITERAL, true and false.

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@JohnCSimon JohnCSimon 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 Oct 5, 2019
@JohnCSimon
Copy link
Member

Hello from triage.
@estebank @petrochenkov
From what I can tell this the comments have been addressed and this is ready for review.

Thanks.

@petrochenkov
Copy link
Contributor

@estebank
Could you move the language change (#64700 (comment)) without any further diagnostic/recovery enhancements into a separate PR?
(Squashing and avoiding stray formatting changes would also be nice.)

@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 Oct 5, 2019
@JohnCSimon
Copy link
Member

Pinging again from triage.
@estebank This has sat idle for a few days, can you address @petrochenkov 's comment?

@joelpalmer joelpalmer added S-inactive and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 21, 2019
@joelpalmer
Copy link

Ping from Triage: Hi @estebank, closing due to inactivity, please reopen when you have updates. Thanks for the PR.

@joelpalmer joelpalmer closed this Oct 21, 2019
@crlf0710
Copy link
Member

@rustbot modify labels to -S-inactive +S-inactive-closed

@rustbot rustbot added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-inactive labels Mar 29, 2020
@estebank
Copy link
Contributor Author

where is_whitelisted is {, -, LITERAL, true and false.

@petrochenkov I started looking at this again and I am a bit confused. Should foo::< 40 + 2 >() not be accepted? Because I think the code I have can reliably deal with it. Do we want to avoid limited look-ahead at all costs? If we don't, then we can "easily" also parse things like foo::< bar && baz >(), foo::< bar - baz >() and foo::< bar.baz() >(). I am slightly concerned on how I am going to come up with a non-hacky way of having bounded lookahead (purely for accurate-ish suggestions) for cases like foo::<bar + baz - quz>, foo::< bar() + 3 > or foo::< bar() > 3 > which start getting parsed as types and fail afterwards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Recover and suggest wrapping in block in const generic value application
10 participants