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

Some floating-point numbers are changed #785

Closed
ErikSchierboom opened this issue Apr 24, 2020 · 8 comments · Fixed by #808
Closed

Some floating-point numbers are changed #785

ErikSchierboom opened this issue Apr 24, 2020 · 8 comments · Fixed by #808

Comments

@ErikSchierboom
Copy link
Contributor

Issue created from fantomas-ui

The value of some floating-point numbers will change when formatting the code. It appears to only apply to negative floating-point numbers:

Code

module Numbers

let x = -3.213f
let y = 1.621f
let z = -1.5f
let a = 3.213f

Result

module Numbers

let x = -3.213000059f
let y = 1.621f
let z = -1.5f
let a = 3.213f

Options

Fantomas 3.2.0

Name Value
IndentOnTryWith false
IndentSpaceNum 4
KeepNewlineAfter false
MaxIfThenElseShortWidth 40
PageWidth 120
ReorderOpenDeclaration false
SemicolonAtEndOfLine false
SpaceAfterComma true
SpaceAfterSemicolon true
SpaceAroundDelimiter true
SpaceBeforeArgument true
SpaceBeforeColon false
StrictMode false
@ErikSchierboom ErikSchierboom changed the title [Bug report from fantomas-ui] Some floating-point numbers are changed Some floating-point numbers are changed Apr 24, 2020
@ErikSchierboom
Copy link
Contributor Author

I've dug into this a bit more, and it looks like this is not Fantomas' doing but FSC: https://fsprojects.github.io/fantomas-tools/#/ast?data=N4KABGBEAmCmBmBLAdrAzpAXFSAacUiaAYmolmPAIYA2as%2BEkAxgPZwWQ2wAuYAHmAC8YALQBmAHQAmAIzj4kEAF8gA

@nojaf nojaf reopened this May 3, 2020
@nojaf
Copy link
Contributor

nojaf commented May 3, 2020

Yes, there seems to be a problem in the FCS indeed, however, there is also a Fantomas one here I believe.

Numbers can't be trusted from the AST anyways, so we use trivia to set the correct number, which in this case also didn't happen correctly.

@nojaf
Copy link
Contributor

nojaf commented May 3, 2020

image

image

We detected the number, the problem is somewhere around the assignment of the trivia to a trivia node.

Given this insight @ErikSchierboom, could I persuade you to tackle this issue yourself?

@nojaf
Copy link
Contributor

nojaf commented May 3, 2020

I believe the problem is located in here.
The minus sign is part of the SynConst range and that is why the match is not found.

Proposed solution:
extend TokenParser to have a case of MINUS + NUMBER TOKEN.

Something along the lines:

| minus::head::rest when (minus.TokenInfo.TokenName = "MINUS" && isNumber head) ->
...

@ErikSchierboom
Copy link
Contributor Author

Yeah sure! It will be my pleasure :) I'll work on this tomorrow.

@nojaf
Copy link
Contributor

nojaf commented May 3, 2020

Great to hear! I recommend you watch https://www.youtube.com/watch?v=FCrkUqCCzgI&list=PLvw_J2kfZCX3Mf6tEbIPZXbzJOD1VGl4K first to have some understanding of what all this trivia fuzz is about.

If you were to have any questions, please reach out over here or on the F# foundation slack.

@ErikSchierboom
Copy link
Contributor Author

Will do! Thanks for the links.

@ErikSchierboom
Copy link
Contributor Author

@nojaf I've submitted a PR to fix this. I have one question about how to best implement it, which I've left as a comment in the PR.

ErikSchierboom added a commit to ErikSchierboom/fantomas that referenced this issue May 4, 2020
ErikSchierboom added a commit to ErikSchierboom/fantomas that referenced this issue May 4, 2020
ErikSchierboom added a commit to ErikSchierboom/fantomas that referenced this issue May 4, 2020
ErikSchierboom added a commit to ErikSchierboom/fantomas that referenced this issue May 4, 2020
@nojaf nojaf closed this as completed in #808 May 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants