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

[1.38] Placement new syntax no longer parses #62617

Closed
Mark-Simulacrum opened this issue Jul 12, 2019 · 13 comments
Closed

[1.38] Placement new syntax no longer parses #62617

Mark-Simulacrum opened this issue Jul 12, 2019 · 13 comments
Labels
A-parser Area: The parsing of Rust source code to an AST regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@Mark-Simulacrum
Copy link
Member

root: mbox 0.3.1; cc @kennytm causes:

  • resin-os/healthdog-rs: start v. end; cc @willnewton

root: rayon-hash 0.2.0 cc @cuviper

  • coder543/xray: start v. end; cc @coder543

Root cause of these on our side is #60803 which removed support for parsing placement new even under #[cfg(false)] and the like. The crater run done there seems to have not found these -- not sure why. We might be testing more now than we were before.

@Mark-Simulacrum Mark-Simulacrum added A-parser Area: The parsing of Rust source code to an AST 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. labels Jul 12, 2019
@kennytm
Copy link
Member

kennytm commented Jul 12, 2019

The <- support in mbox has been removed in 0.5.0.

The mbox 0.3.1 dependency was introduced by systemd 0.1.0, but the current version is systemd 0.4.0 which doesn't depend on mbox.

resin-os/healthdog-rs depends on systemd 0.* so a cargo update is enough to remove the broken mbox dependency.

FYI @willnewton you may want to run cargo update to update the Cargo.lock.

@cuviper
Copy link
Member

cuviper commented Jul 12, 2019

root: rayon-hash 0.2.0 cc @cuviper

  • coder543/xray: start v. end; cc @coder543

rayon-hash came up in #50832 and was fixed in 0.3.0. It's up to 0.5.0 now, and deprecated in favor of hashbrown/rayon anyway. @coder543 should update, if this xray project is at all active.

@coder543
Copy link

coder543 commented Jul 12, 2019

@cuviper xray is unfortunately not being developed any further at this time

@coder543
Copy link

@Mark-Simulacrum those "ccs" you tried to do to tag people don't work because you put them in code blocks, just FYI...

@cuviper
Copy link
Member

cuviper commented Jul 12, 2019

@coder543 would you accept a PR bumping dependencies? Otherwise, maybe we should remove this repo from crater's list.

@coder543
Copy link

@cuviper yeah, either option would be fine!

@Mark-Simulacrum
Copy link
Member Author

@Mark-Simulacrum those "ccs" you tried to do to tag people don't work because you put them in code blocks, just FYI...

That's intentional, I try to not ping downstream users before the root cause is assessed and/or addressed by that author.

@coder543
Copy link

Updated code has been merged

@nagisa
Copy link
Member

nagisa commented Jul 14, 2019

Since this syntax was never proposed for stablisation, I think it is fine to say it is an oversight/bug this unstable syntax was not gated.

If we decide we cannot feasibly to keep unstable syntax properly gated (because it would not work with cfg, then <- parsing should be restored.

@nagisa
Copy link
Member

nagisa commented Jul 14, 2019

We should decide how we want to deal with unstable syntax.

If unstable syntax like in this particular instance the <- operator, was properly gated, people would have to always use nightly if any code uses features with unstable syntax, even if such uses appeared behind #[cfg]. The current situation ended up not gating the syntax properly which is now causing problems when changing/removing it. Tagging T-lang to make them aware of it.

@nagisa nagisa added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Jul 14, 2019
@petrochenkov
Copy link
Contributor

I try to ensure during reviews that new features changing the grammar are gated at parse time.

@Mark-Simulacrum
Copy link
Member Author

Removing T-compiler and nominating for T-lang -- I think we should close this; the ungated syntax landed a long time ago and we're pretty good these days about ensuring that we check grammar early enough that this isn't a problem (as @petrochenkov notes).

@Mark-Simulacrum Mark-Simulacrum added I-nominated and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 28, 2019
@Centril
Copy link
Contributor

Centril commented Aug 1, 2019

Me, @cramertj, and @qmx agree with @Mark-Simulacrum's rationale and so we'll close this as resolved.

@Centril Centril closed this as completed Aug 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-parser Area: The parsing of Rust source code to an AST regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants