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

RFC: require parentheses for chained comparisons #558

Merged

Conversation

dgrunwald
Copy link
Contributor

@sinistersnare
Copy link

I like this, I wonder how other people will react :)

@SimonSapin
Copy link
Contributor

+1. I don’t worry too much about the current behavior being unexpected, but I’d like the option to add Python-style chained comparisons in the future.

@Kimundi
Copy link
Member

Kimundi commented Jan 7, 2015

I'm in favour of this, simply because it increases the number of options we can take post 1.0, and is unlikely to have much fallout.

Personally, I'd use parenthesis anyway in any case where I'd wanted to code like the one affected by this change, so I don't see it as a loss.

@dgrunwald
Copy link
Contributor Author

While testing my implementation for this, I found an unexpected interaction with another part of the language: generic syntax.

It turns out when compiling

fn f<T>() {}

fn main() {
    f<i32>();
}

the error message changes from "error: unresolved name i32" to "Error: Chained comparison operators require parentheses".
It should be trivial to output an additional message in the case of > being chained after <:
"Help: Use ::< instead of < if you meant to specify type arguments."

Mention that this doesn't break rustc.
@dgrunwald
Copy link
Contributor Author

I have implemented this RFC: https://github.com/dgrunwald/rust/compare/require-parens-for-chained-comparison
Looks like the breaking change did not affect any code in rustc. The only code that needed adjustment was test/compile-fail/unsized2.rs, due to the additional diagnostics for the existing syntax error.

@huonw
Copy link
Member

huonw commented Jan 7, 2015

This looks like a good idea, for all the reasons given: correctness, Python style chaining and better a<b>(c) errors.

@huonw huonw self-assigned this Jan 7, 2015
@huonw
Copy link
Member

huonw commented Jan 7, 2015

This is likely to be merged before alpha, I'm just waiting on feedback from @nikomatsakis. Thanks for writing the patch already, @dgrunwald!

(I see from your github profile that you're in Europe, meaning it's nighttime; so I'll likely make the PR with your commit and manage getting it merged, unless you are in fact awake. 😄 )

@nikomatsakis
Copy link
Contributor

@huonw r+

@nrc
Copy link
Member

nrc commented Jan 8, 2015

I approve of the idea and I realise it is just before the alpha, but merging a new RFC in 16 hours and with 3 comments is not really in the spirit of the RFC process.

@huonw
Copy link
Member

huonw commented Jan 8, 2015

Oh I forgot to write a comment with the thinking. Thank you for the prompt, @nick29581.

All the (admittedly few) comments are positive, and I got feedback from most of core team. Everyone was in favour of merging this for the alpha as a conservative step that also had concrete immediate improvements (the error messages for C++-style type hints are much better). The intention is to revisit and possibly revert in future. I opened #561.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-expressions Term language related proposals & ideas A-syntax Syntax related proposals & ideas
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants