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

Disallow underscore suffix for string-like literals. #41990

Conversation

qnighy
Copy link
Contributor

@qnighy qnighy commented May 14, 2017

This patch turns string/bytestring/char/byte literals followed by an underscore, like "Foo"_, to an error.

scan_optional_raw_name will parse _ as a valid raw name, but it will be rejected by the parser. I also considered just stopping parsing when the suffix is _, but in that case "Foo"_ will be lexed as two valid tokens.

Fixes the latter half of #41723.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pnkfelix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@Mark-Simulacrum Mark-Simulacrum added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 14, 2017
@Mark-Simulacrum
Copy link
Member

Thanks for the PR! We'll periodically check in to make sure that @pnkfelix or someone else from the team review it soon.

@shepmaster
Copy link
Member

I've pinged @pnkfelix on IRC as a reminder to review this PR. Fear not, @qnighy, you will get your review! 😺

@withoutboats
Copy link
Contributor

withoutboats commented May 20, 2017

I was curious what the original cause of this was, since it seems like the compiler explicitly allowed it, and here it is (2014!). This was intended to just be future proofing for literal suffixes (probably so you could theoretically write custom string literal types). It looks like _ was singled out to act as if it had no suffix (which makes sense), but that incidentally enabled it to parse even though the feature doesn't exist.

@arielb1
Copy link
Contributor

arielb1 commented May 23, 2017

r? @nrc

@nrc
Copy link
Member

nrc commented May 23, 2017

@qnighy Since this is technically a breaking change, it would be good to make this into a warning for a 6-week cycle before making it an error. It looks like that shouldn't be too hard to do here - you'd need to emit a warning in the _ branch of the if expression here.

@Mark-Simulacrum Mark-Simulacrum 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 May 28, 2017
@qnighy
Copy link
Contributor Author

qnighy commented May 31, 2017

@nrc It seems that we have to delay the check to ast_validation because syntax doesn't know rustc_lint?

@nrc
Copy link
Member

nrc commented May 31, 2017

Maybe someone else on @rust-lang/compiler will correct me, but I think it is fine to hard-wire a warning rather than use lints.

@qnighy
Copy link
Contributor Author

qnighy commented May 31, 2017

It looks harder if we have to use lints instead of warnings. We would have to propagate suffix information to AST.

@qnighy
Copy link
Contributor Author

qnighy commented May 31, 2017

OK, I will try to implement it using warnings.

@qnighy
Copy link
Contributor Author

qnighy commented May 31, 2017

Implemented the warning cycle #42326 without lint. This would be sufficient unless lint is preferred.

@nikomatsakis
Copy link
Contributor

Hmm, lints are preferred, but I agree that will be difficult since this message appears so early (in the lexer). I can live with the "hard warning" you have here. It does mean that we don't have the option the "deny by default" half-step -- we just have to move to a hard error, but that seems ok in this case.

(I guess we could continue issuing a warning if "cap-lints" is enabled, but that feels unnecessary).

@Mark-Simulacrum
Copy link
Member

@nrc @nikomatsakis This looks like it's ready to go -- is that correct? I don't want to r+ since there may be some unresolved questions.

@nikomatsakis
Copy link
Contributor

I agree, this is good to go.

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jun 5, 2017

📌 Commit 0b8c3de has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Jun 6, 2017

⌛ Testing commit 0b8c3de with merge e862695...

bors added a commit that referenced this pull request Jun 6, 2017
…ike-literals, r=nikomatsakis

Disallow underscore suffix for string-like literals.

This patch turns string/bytestring/char/byte literals followed by an underscore, like `"Foo"_`, to an error.

`scan_optional_raw_name` will parse `_` as a valid raw name, but it will be rejected by the parser. I also considered just stopping parsing when the suffix is `_`, but in that case `"Foo"_` will be lexed as two valid tokens.

Fixes the latter half of #41723.
@bors
Copy link
Contributor

bors commented Jun 6, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing e862695 to master...

@bors bors merged commit 0b8c3de into rust-lang:master Jun 6, 2017
@qnighy qnighy deleted the disallow-underscore-suffix-for-string-like-literals branch June 7, 2017 03:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants