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

Corrected EBNF grammar for from_str #97498

Merged
merged 1 commit into from
Jun 1, 2022
Merged

Corrected EBNF grammar for from_str #97498

merged 1 commit into from
Jun 1, 2022

Conversation

ijchen
Copy link
Contributor

@ijchen ijchen commented May 28, 2022

Hello! This is my first time contributing to an open-source project. I'm excited to have the chance to contribute to the rust community 🥳

I noticed an issue with the documentation for from_str in f32 and f64. It states that "All strings that adhere to the following EBNF grammar when lowercased will result in an Ok being returned. I believe this is incorrect for the string ".", which is valid for the given EBNF grammar, but does not result in an Ok being returned (playground). I have simplified the grammar in a way which fixes that, but is otherwise identical.

Previously, the Number part of the EBNF grammar had an option for '.' Digit*, which would include the string ".". This is not valid, and does not return an Ok as stated. The corrected version removes this, and still allows for the '.' Digit+ case with the already existing Digit* '.' Digit+ case.

Previously, the `Number` part of the EBNF grammar had an option for `'.' Digit*`, which would include the string "." (a single decimal point). This is not valid, and does not return an Ok as stated. The corrected version removes this, and still allows for the `'.' Digit+` case with the already existing `Digit* '.' Digit+` case.
@rust-highfive
Copy link
Collaborator

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label May 28, 2022
@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 @Mark-Simulacrum (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 28, 2022
@Stargateur
Copy link
Contributor

Stargateur commented May 28, 2022

This problem have been added in #94838, it's a little strange now cause EBNF is a CASE sensitive syntax. roolback the commit probably make sense @Dylan-DPC

Specially it's added "infinity" that was not mandatory before.

@ijchen
Copy link
Contributor Author

ijchen commented May 28, 2022

Okay, would you like me to close this PR?

@ijchen
Copy link
Contributor Author

ijchen commented May 28, 2022

For what it's worth, I do think there is some value to #94838 making it clear that from_str is case-insensitive, and that "infinity" is valid. Both of these seem intentional from parse_partial_inf_nan in dec2flt/parse.rs

I don't know what the process is for updating the docs in a way that removes something that has been mandatory, but I'd imagine that it could be potentially problematic to remove existing documentation of behavior that is currently implemented (and therefore possibly in-use). But maybe that is not really a concern

@Stargateur
Copy link
Contributor

At the very least the syntax should correct the case thing cause EBNF is clear about the case, maybe use ABNF would be more clear than break EBNF rule.

@Mark-Simulacrum
Copy link
Member

It seems like this change is correct, right? There's probably some further changes that could be made to improve things, but I don't think it's a good idea to wait until those larger changes are implemented.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jun 1, 2022

📌 Commit 0484cfb has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 1, 2022
@Stargateur
Copy link
Contributor

Stargateur commented Jun 1, 2022

@ijchen Do you want to handle the following, create issue etc... (thus maybe just a PR is enough) or do you want I do it ? Basically, I think we need to switch to ABNF that is more easy and have more documentation and tool and is a lot more used than EBNF. We also need to confirm with std team what they want f64::from_str() to officially handle

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jun 1, 2022
Corrected EBNF grammar for from_str

Hello! This is my first time contributing to an open-source project. I'm excited to have the chance to contribute to the rust community 🥳

I noticed an issue with the documentation for `from_str` in `f32` and `f64`. It states that "All strings that adhere to the following [EBNF](https://www.w3.org/TR/REC-xml/#sec-notation) grammar when lowercased will result in an `Ok` being returned. I believe this is incorrect for the string `"."`, which is valid for the given EBNF grammar, but does not result in an `Ok` being returned ([playground](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=09f891aa87963a56d3b0d715d8cbc2b4)). I have simplified the grammar in a way which fixes that, but is otherwise identical.

Previously, the `Number` part of the EBNF grammar had an option for `'.' Digit*`, which would include the string `"."`. This is not valid, and does not return an Ok as stated. The corrected version removes this, and still allows for the `'.' Digit+` case with the already existing `Digit* '.' Digit+` case.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 1, 2022
Rollup of 9 pull requests

Successful merges:

 - rust-lang#94647 (Expose `get_many_mut` and `get_many_unchecked_mut` to HashMap)
 - rust-lang#97216 (Ensure we never consider the null pointer dereferencable)
 - rust-lang#97399 (simplify code of finding arg index in `opt_const_param_of`)
 - rust-lang#97470 (rustdoc: add more test coverage)
 - rust-lang#97498 (Corrected EBNF grammar for from_str)
 - rust-lang#97562 (Fix comment in `poly_project_and_unify_type`)
 - rust-lang#97580 (Add regression test for rust-lang#71546)
 - rust-lang#97611 (Tweak insert docs)
 - rust-lang#97616 (Remove an unnecessary `Option`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@ijchen
Copy link
Contributor Author

ijchen commented Jun 1, 2022

@Stargateur I don't mind doing it, it would be nice to get some experience with this process. Thank you though! I definitely agree that we should eventually confirm exactly what f32/f64::from_str() should officially support in documentation. The changes I made are aimed at removing only the obviously incorrect part of the documentation (considering "." a valid float).

@Mark-Simulacrum yes, as far as I can tell this change is correct (well, unless f32/f64::from_str() actually should consider "." a valid float). I definitely agree that its preferable that this is fixed sooner, even if there are still further improvements that could be made. I'm new to this process - are you guys waiting on me for something now? EDIT: Just googled what "r+ rollup" did, so I see you are not waiting on me.

@bors bors merged commit e1d2e65 into rust-lang:master Jun 1, 2022
@rustbot rustbot added this to the 1.63.0 milestone Jun 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants