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

Idempotency problem when using a list with interpolated strings #2242

Closed
3 tasks done
abelbraaksma opened this issue May 10, 2022 · 9 comments
Closed
3 tasks done

Idempotency problem when using a list with interpolated strings #2242

abelbraaksma opened this issue May 10, 2022 · 9 comments

Comments

@abelbraaksma
Copy link
Member

abelbraaksma commented May 10, 2022

Issue created from fantomas-online

Before formatting, code was compiling:

let f hdrKey =
    readOnlyDict[$"{hdrKey}:0", "X-MessageID"
                 $"{hdrKey}:1", "Authorization"]

Formatted code

let f hdrKey =
    readOnlyDict[$"{hdrKey}:0", "X-MessageID"
    $"{hdrKey}:1", "Authorization"]

Reformatted code (note the missing semicolon!)

let f hdrKey =
    readOnlyDict[$"{hdrKey}:0", "X-MessageID" $"{hdrKey}:1", "Authorization"]

Problem description

Fantomas was not able to produce the same code after reformatting the result.

Extra information

  • The formatted result breaks my code.
  • The formatted result gives compiler warnings errors.
  • I or my company would be willing to help fix this.

Options

Fantomas master branch at 2022-05-09T12:59:04Z - f2e3c08

Default Fantomas configuration

  • I've seen this fail with Fantomas 4.7.0
  • Exactly the same code with VS 2022 Extension uses Fantomas 4.6.0 and succeeds and leaves the code as-is
  • In other words, seems to be a regression.

Did you know that you can ignore files when formatting from fantomas-tool or the FAKE targets by using a .fantomasignore file? yes ;).

@abelbraaksma
Copy link
Member Author

abelbraaksma commented May 10, 2022

Alternative, failing code:

    let f x = readOnlyDict[$"{x}:0", ""; $"{x}:1", ""]

After formatting, breaks:

let f x =
    readOnlyDict[$"{x}:0", ""
    $"{x}:1", ""]

This seems related to #2201, where it was suggested that adding a space before the list bracket fixes Fantomas' handling. However, in many cases adding a space will break the code, as f x[] is not equal to f x [] in F#.

@nojaf
Copy link
Contributor

nojaf commented May 10, 2022

Hello, thank you for raising this.
I left feedback on how to fix this in #2158.
Are you interested in submitting a PR?

@nojaf
Copy link
Contributor

nojaf commented May 10, 2022

In other words, seems to be a regression.

That is debatable btw. In F# 6 the new indexer syntax recontextualizes what you wrote.

> readOnlyDict;;
val it:
  (seq<'a * 'b> -> Collections.Generic.IReadOnlyDictionary<'a,'b>)
    when 'a: equality

readOnlyDictis a function call, so it is weird that you are trying to index into it with the $"{x}:0", ""; $"{x}:1", "" expression. Fantomas does not know what your code is doing, so it saw an index expression and restored it as such.

The only bug I see here is how it is wrongly handling a multiline expression inside the index.

@abelbraaksma
Copy link
Member Author

abelbraaksma commented May 10, 2022

That is debatable btw. In F# 6 the new indexer syntax recontextualizes what you wrote.

@nojaf Well, I mentioned that after the observation that Fantomas 4.6 tackles this code correctly somehow. The project is F# 6.0 indeed, but running it with commandline Fantomas 4.7 showed the problem.

so it is weird that you are trying to index into it with the $"{x}:0", ""; $"{x}:1", "" expression.

This is an interesting observation. I didn't consider this. So I tried a few things in F# Interactive, and I get a compile error each time, esp. w.r.t. it being an indexed property.

But, in the "normal" .fs file, it gets compiled just fine. Here's what I simplified it to, which compiles fine in a .NET 6.0 project, but does NOT compile when send to Interactive:

let configureCorsPolicy x = Seq.iter ignore x
let hdrKey = "foo"

configureCorsPolicy
    readOnlyDict[$"{hdrKey}:0", "X-MessageID"; $"{hdrKey}:1", "Authorization"]

I thought this was similar to the subtle difference between g().ToString() and g ().ToString(), but it seems it is something else entirely.

@abelbraaksma
Copy link
Member Author

abelbraaksma commented May 10, 2022

Sorry, I stand corrected. It wasn't an error in FSI, it was a warning:

info FS3365: The syntax 'expr1[expr2]' is used for indexing. Consider adding a type annotation to enable indexing, or if calling a function add a space, e.g. 'expr1 [expr2]'.

Darn, and now I notice the #nowarn "3365". We need to get rid of that!

@nojaf
Copy link
Contributor

nojaf commented May 10, 2022

Thanks for following up on this.

@abelbraaksma
Copy link
Member Author

The compiler is a bit weird here. I removed the #nowarn, upped the warning level to 5, but it doesn't report on it, except for in the editor itself when hovering. This is odd, since I did get the warning with a slightly different syntax.

That's clearly F# compiler territory and not so much Fantomas territory.

I might have a look one of these days in submitting a PR to fix the underlying bug where the alignment gets screwed up in multiline statements.

@nojaf
Copy link
Contributor

nojaf commented May 10, 2022

The compiler is a bit weird here

Well, one other thing to note is that Fantomas is using a slightly newer version of the compiler bits.
In the past, we referenced the FCS from NuGet, so depending on your .NET SDK version, there might be a difference there.

For a few days now, we are using our own FCS version, which is tied closely to the main branch of dotnet/fsharp. (See #2217).

All of this isn't really a problem unless there are some more significant changes like the new index syntax.

I might have a look one of these days in submitting a PR to fix the underlying bug where the alignment gets screwed up in multiline statements.

Thank you, check out our Contribution Guidelines to get started.

@nojaf
Copy link
Contributor

nojaf commented Aug 24, 2022

Fixed by #2446

@nojaf nojaf closed this as completed Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants