-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Update ?
repetition disambiguation.
#49719
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
I was going to say that disallowing macro_rules! test {
($($t: ident)**) => {
println!("{}", stringify!($($t)**));
}
}
fn main() {
let (a, b, c) = (1, 2, 3);
println!("{}", test!(a * b * c));
} Which yields a very weird error. Using |
@clarcharr Not weird at all, actually. :-) Since allowing |
@alexreg I agree, but the error in no way indicates that the separator is disallowed. It simply parses expecting In hindsight, this makes a lot of sense, but the errors could definitely be improved. I agree that disallowing |
@clarcharr Hmm. I see your point, but I still think the error message is essentially good as-is. Maybe what we need is a note attached to the existing error message when a match fails and it detects |
@bors try Please schedule a check-only crater run. |
Update `?` repetition disambiguation. **Do not merge** (yet) This is a test implementation of some ideas from discussion in #48075 . This PR - disallows `?` repetition from taking a separator, since the separator is never used. - disallows the use of `?` as a separator. This allows patterns like `$(a)?+` to match `+` and `a+` rather than `a?a?a`. This is a _breaking change_, but maybe that's ok? Perhaps a crater run is the right approach? cc @durka @alexreg @nikomatsakis
☀️ Test successful - status-travis |
How do I do that? |
@mark-i-m That's only a message to the people who run the crater 😅 |
A check-only crater run has been commenced; ETA is somewhat unknown but probably less than 5 days. |
Crater results are at: http://cargobomb-reports.s3.amazonaws.com/pr-49719/index.html. These results show 0 regressions; crater is green. Note that Crater does not test crates on Windows. (interested observers: Crater is a tool for testing the impact of changes on the crates.io ecosystem. You can find out more at the repo if you're curious) |
@bors r+ |
📌 Commit 54bba4c has been approved by |
Update `?` repetition disambiguation. **Do not merge** (yet) This is a test implementation of some ideas from discussion in #48075 . This PR - disallows `?` repetition from taking a separator, since the separator is never used. - disallows the use of `?` as a separator. This allows patterns like `$(a)?+` to match `+` and `a+` rather than `a?a?a`. This is a _breaking change_, but maybe that's ok? Perhaps a crater run is the right approach? cc @durka @alexreg @nikomatsakis
Personally I'd prefer if we implement this as a deprecation warning so we can change the behavior in a future edition instead of risking to break existing behavior. We shouldn't break things without warning if there is not a big reason to do that (such as fixing soundness holes). |
Clippy broke. I think I fixed it with some feature gate. |
Oh, hmm... That's good to know... Maybe the edition would be the best way to turn this on... |
@mark-i-m How do editions work? Would this feature only be enabled for Rust 2018 code then? |
I think that would lead to the least confusion. We would warn now and then make it a hard error in the new edition. Then we could stabilize the feature some time in the 2018 edition... Some other options are
Both of these alternatives seem less good than just waiting a few months and deprecating the old behavior... |
I’m not against this. Sounds fair. |
I opened #51336 as a potential fix in the case we decide to roll this back for the 2015 edition without a feature gate. |
I think my preference would be that we do the following:
@mark-i-m would you be up for preparing a "straight" revert PR to give us time to revisit this question? |
(I filed #51416 to track a revert; we should still discuss the right thing to do afterwards. I personally like the idea of phasing this in with the new edition.) |
…tsakis Revert rust-lang#49719 This also needs to be backported into beta. Fixes rust-lang#51416. r? @nikomatsakis
Rollup of 9 pull requests Successful merges: - #51186 (Remove two redundant .nll.stderr files) - #51283 (Deny #[cfg] and #[cfg_attr] on generic parameters.) - #51368 (Fix the use of closures within #[panic_implementation]) - #51380 (Remove dependency on fmt_macros from typeck) - #51389 (rustdoc: Fix missing stability and src links for inlined external macros) - #51399 (NLL performance boost) - #51407 (Update RLS and Rustfmt) - #51417 (Revert #49719) - #51420 (Tries to address the recent network issues) Failed merges:
My rule of thumb is that if it's something useful long-term and has <5 crates.io regressions then we should do it. |
@petrochenkov No worries. I agree with your thinking. We need to re-implement this patch for the 2018 edition at least, I think. |
Request for comments/FCP on newest implementation: #51934 |
Do not merge (yet)
This is a test implementation of some ideas from discussion in #48075 . This PR
?
repetition from taking a separator, since the separator is never used.?
as a separator. This allows patterns like$(a)?+
to match+
anda+
rather thana?a?a
. This is a breaking change, but maybe that's ok? Perhaps a crater run is the right approach?cc @durka @alexreg @nikomatsakis