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

move some invalid exponent detection into rustc_session #131656

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

richard-uk1
Copy link
Contributor

@richard-uk1 richard-uk1 commented Oct 13, 2024

This PR moves part of the exponent checks from rustc_lexer/rustc_parser into rustc_session.

This change does not affect which programs are accepted by the complier, or the diagnostics that are reported, with one main exception. That exception is that floats or ints with suffixes beginning with e are rejected after the token stream is passed to proc macros, rather than being rejected by the parser as was the case. This gives proc macro authors more consistent access to numeric literals: currently a proc macro could interpret 1m or 30s but not 7eggs or 3em. After this change all are handled the same. The lexer will still reject input if it contains e followed by a number, +/-, or _ if they are not followed by a valid integer literal (number + _), but this doesn't affect macro authors who just want to access alpha suffixes.

This PR is a continuation of #79912. In that PR, it was suggested that a new enum was used to indicate type of exponent (whether accepted or rejected). I originally took that approach with this PR, but it didn't seem necessary and made the changes more complex. I can try to go down that road instead if that's the consensus. It is also solving exactly the same problem as #111628.

TODO before ready for review (assuming approach is OK)

  • Some more UI tests (e.g. 1em)
  • Check that re-ordering of errors is OK, or how to change it. If OK, update output.
  • Consider improving UI for suffixes beginning with e (if suffix begins with 'e' suggest an exponential)
  • Currently if the character following the e is _, then the lexer tries to parse an exponent and fails if there are no digits after. The issue is that a valid integer can have any number of _s before the digit, meaning deciding whether a the suffix is a number or not requires unbounded lookahead. There are a few options here This now handles arbitrary _s, removing the special case.

Although I haven't marked this PR as 'ready for review' since there are outstanding stuff that need doing, I do want to get feedback on the approach. Ready for review now.

Also do you want me to write an MCP

r: @petrochenkov, since they reviewed #79912.

@rustbot
Copy link
Collaborator

rustbot commented Oct 13, 2024

r? @petrochenkov

rustbot has assigned @petrochenkov.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 13, 2024
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@richard-uk1 richard-uk1 force-pushed the move_empty_exponent_to_rustc_session branch from 8b41315 to e8c244e Compare October 13, 2024 23:06
@rust-log-analyzer

This comment has been minimized.

@richard-uk1 richard-uk1 force-pushed the move_empty_exponent_to_rustc_session branch from a642bff to 82a8103 Compare November 10, 2024 12:06
@rust-log-analyzer

This comment has been minimized.

@richard-uk1 richard-uk1 marked this pull request as ready for review November 10, 2024 12:56
@richard-uk1
Copy link
Contributor Author

I've added some ui tests and I think this is now ready for review. It doesn't stop @petrochenkov changing the whole thing later if they so choose.

@rust-log-analyzer

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants