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 leading underscores in float exponents. #114567

Conversation

nnethercote
Copy link
Contributor

Such as 1e_3, 1E+__3, 1e-_________3_3.

  • They are ugly and never used in practice. (The test suite and compiler code have no examples of them.)
  • They don't match normal decimal literals. (You can't write x = _3;.)
  • They complicate attempts to allow integers with suffixes beginning with e, such as 1em (currently disallowed, but desired in Allow numeric tokens containing 'e' that aren't exponents be passed to proc macros #111615). Because when given a char sequence like 1e the lexer must decide whether what follows the e is a decimal integer (in which case it's a float with exponent) or something else (in which case it's an integer with a suffix). But unbounded char lookahead is required to get past the possibly unlimited number of leading underscores. Disallowing the leading underscores reduces the lookahead to two: one for a possible +/-, and then one more for a digit or non-digit.

r? @ghost

Such as `1e_3`, `1E+__3`, `1e-_________3_3`.

- They are ugly and never used in practice. (The test suite and compiler
  code have no examples of them.)
- They don't match normal decimal literals. (You can't write `x = _3;`.)
- They complicate attempts to allow integers with suffixes beginning
  with `e`, such as `1em` (currently disallowed, but desired in
  rust-lang#111615). Because when given a char sequence like `1e` the lexer must
  decide whether what follows the `e` is a decimal integer (in which
  case it's a float with exponent) or something else (in which case it's
  an integer with a suffix). But unbounded char lookahead is required to
  get past the possibly unlimited number of leading underscores.
  Disallowing the leading underscores reduces the lookahead to two: one
  for a possible `+`/`-`, and then one more for a digit or non-digit.
@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 Aug 6, 2023
@nnethercote nnethercote marked this pull request as draft August 6, 2023 23:16
@nnethercote
Copy link
Contributor Author

cc @petrochenkov

@petrochenkov petrochenkov self-assigned this Aug 7, 2023
@petrochenkov
Copy link
Contributor

@bors try

@bors
Copy link
Contributor

bors commented Aug 7, 2023

⌛ Trying commit 99c21f2 with merge 77c31533f5de739620341d1e55d79a35cbb15b78...

@petrochenkov petrochenkov added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 7, 2023
@bors
Copy link
Contributor

bors commented Aug 7, 2023

☀️ Try build successful - checks-actions
Build commit: 77c31533f5de739620341d1e55d79a35cbb15b78 (77c31533f5de739620341d1e55d79a35cbb15b78)

@petrochenkov
Copy link
Contributor

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-114567 created and queued.
🤖 Automatically detected try build 77c31533f5de739620341d1e55d79a35cbb15b78
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🚧 Experiment pr-114567 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-114567 is completed!
📊 95 regressed and 4 fixed (343114 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Aug 10, 2023
@nnethercote
Copy link
Contributor Author

I always have trouble reading crater results, but there are at least two genuine regressions.

liquid-core:

[INFO] [stderr]     Checking liquid-core v0.26.4 (/opt/rustwide/workdir)
[INFO] [stdout] error: expected at least one digit in exponent
[INFO] [stdout]    --> src/model/value/ser.rs:675:50
[INFO] [stdout]     |
[INFO] [stdout] 675 |         let actual = crate::model::Value::scalar(3.14e_10f64);
[INFO] [stdout]     |                                                  ^^^^^^^^^^^
[INFO] [stdout] 
[INFO] [stdout] 
[INFO] [stdout] error: expected at least one digit in exponent
[INFO] [stdout]    --> src/model/value/ser.rs:675:50
[INFO] [stdout]     |
[INFO] [stdout] 675 |         let actual = crate::model::Value::scalar(3.14e_10f64);
[INFO] [stdout]     |                                                  ^^^^^^^^^^^
[INFO] [stdout] 
[INFO] [stdout] 
[INFO] [stdout] error: invalid suffix `_10f64` for float literal
[INFO] [stdout]    --> src/model/value/ser.rs:675:50
[INFO] [stdout]     |
[INFO] [stdout] 675 |         let actual = crate::model::Value::scalar(3.14e_10f64);
[INFO] [stdout]     |                                                  ^^^^^^^^^^^ invalid suffix `_10f64`
[INFO] [stdout]     |
[INFO] [stdout]     = help: valid suffixes are `f32` and `f64`

liquid-value:

[INFO] [stdout] error: expected at least one digit in exponent
[INFO] [stdout]   --> tests/value.rs:19:46
[INFO] [stdout]    |
[INFO] [stdout] 19 |     let actual = liquid_value::Value::scalar(3.14e_10f64);
[INFO] [stdout]    |                                              ^^^^^^^^^^^
[INFO] [stdout] 
[INFO] [stdout] 
[INFO] [stdout] error: invalid suffix `_10f64` for float literal
[INFO] [stdout]   --> tests/value.rs:19:46
[INFO] [stdout]    |
[INFO] [stdout] 19 |     let actual = liquid_value::Value::scalar(3.14e_10f64);
[INFO] [stdout]    |                                              ^^^^^^^^^^^ invalid suffix `_10f64`
[INFO] [stdout]    |
[INFO] [stdout]    = help: valid suffixes are `f32` and `f64`

In both cases allowing a single leading underscore would be enough.

@nnethercote
Copy link
Contributor Author

Also a surprising number of failures caused by "no space left on device", i.e. disks filling up.

@petrochenkov
Copy link
Contributor

Also a surprising number of failures caused by "no space left on device", i.e. disks filling up.

From what I've seen it always happens with crater runs nowadays.

@petrochenkov
Copy link
Contributor

Wait a second, I completely missed that the implementation is incorrect.
We only need to catch underscore after +/- in exponent, if the +/- is actually present, only then you get identifier tokens in the grammar #111628 (comment).

The 3.14e_10f64 case is still an Int(3) Punct(.) Int(14/e_10f64) and not interesting.
@rustbot author

@rustbot rustbot 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 Aug 11, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 21, 2025
lexer: Disallow some leading underscores in float exponents

A second, scaled down, attempt at rust-lang#114567.
cc rust-lang#111628 (comment)
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. 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.

5 participants