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

Crates using placement syntax behind a cfg broken #50832

Closed
pietroalbini opened this issue May 17, 2018 · 14 comments
Closed

Crates using placement syntax behind a cfg broken #50832

pietroalbini opened this issue May 17, 2018 · 14 comments
Assignees
Labels
C-bug Category: This is a bug. P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Milestone

Comments

@pietroalbini
Copy link
Member

Some crates were using <- behind a feature flag, so they worked on stable even if the placement syntax was nightly-only. After removing it though they fail to compile even on stable with a syntax error, because <- is not recognized anymore.

@pietroalbini pietroalbini added I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. regression-from-stable-to-beta Performance or correctness regression from stable to beta. C-bug Category: This is a bug. labels May 17, 2018
@pietroalbini pietroalbini added this to the 1.27 milestone May 17, 2018
@kennytm
Copy link
Member

kennytm commented May 17, 2018

It is already fixed in mbox 0.5.0 and 0.4.4.

@nikomatsakis
Copy link
Contributor

Oh, that's annoying. We should patch rayon-hash.

@petrochenkov
Copy link
Contributor

This is not the first time this is happening, removal of impl Trait for .. {} caused same consequences (see #46480).
We should probably continue parsing <- and report the error later in AST validation.

@nikomatsakis
Copy link
Contributor

Discussed in the @rust-lang/compiler meeting. I think we reached this consensus:

  • in this case, yes, we should do as @petrochenkov suggests: keep parsing, error in AST validation
  • if in the future, if it happens that we want to repurpose the syntax, we deal with it then: best of course is to use an edition for it, but maybe by then there no longer exist crates using it
  • going forward, it would be ideal if we could do some kind of "pre-cfg / expansion" feature check to try and avoid this in the future; that may or may not be feasible though, and we couldn't do it retroactively for all existing things anyway

@nikomatsakis
Copy link
Contributor

Some discussion of the final at or around here in the IRC logs

https://botbot.me/mozilla/rustc/2018-05-17/?msg=100165163&page=3

@nikomatsakis
Copy link
Contributor

triage: P-high

@nikomatsakis
Copy link
Contributor

Would anyone have time to whip up a change to the parser?

cc @rust-lang/compiler @aidanhs

@kennytm kennytm added the P-high High priority label May 17, 2018
@nikomatsakis
Copy link
Contributor

For reference, the PR that reverted the syntax was #48333

@cuviper
Copy link
Member

cuviper commented May 17, 2018

I am working on rayon-hash. The quick hack would be to just nuke the placement stuff, but I'm trying to do a proper update to catch up to std's code -- fun with git subtree.

But frankly, I don't know of anyone really using rayon-hash anyway.

@cuviper
Copy link
Member

cuviper commented May 17, 2018

Fixed in rayon-hash 0.3.0.

@nikomatsakis nikomatsakis self-assigned this May 23, 2018
@nikomatsakis
Copy link
Contributor

I'll handle this.

@nikomatsakis
Copy link
Contributor

Restored in #51052

@leoyvens
Copy link
Contributor

We could try calling more attention to lib authors about this through the API guidelines.

@estebank
Copy link
Contributor

estebank commented Sep 19, 2018

The two regressions where about <-, but we also reverted the in syntax as well, which has degraded back a couple of errors, like the following:

fn main() {
    let i = ();
    for x in in 0..1 {}
    i;
}

Would it be ok to create a PR removing the in parsing while keeping the current <- handling for a while longer?

CC #36611 #52964 #51602

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants