Skip to content

Conversation

@kerams
Copy link
Contributor

@kerams kerams commented Dec 9, 2022

Expanding on #14370 and fsharp/fslang-suggestions#539.

One minor problem are error messages. Consider cases with inconsistent types:

b9Ye9eyW7X

RatAEk93di

In the latter case, TcExpr seems to insert type casts of some sort (I think), which prevent EvalLiteralExprOrAttribArg from evaluating the expression, so type checking fails before enum case type consistency could be checked and we don't get the nice message in the first picture. I could use TcExprOfUnknownType, but I don't know how to "bind" the returned type to the type variable fieldTy.
Sorted

Also, the parentheses are required, but I think that's fine.

@kerams kerams requested a review from a team as a code owner December 9, 2022 20:19
@T-Gro
Copy link
Member

T-Gro commented Dec 9, 2022

Can you please add a test for the diagnostics just like in the screenshot?

@T-Gro
Copy link
Member

T-Gro commented Dec 9, 2022

The () will be required only in the enum case, right?
Just to verify - would be good to know if Fantomas keeps those, otherwise a change would be needed there as well.

@kerams
Copy link
Contributor Author

kerams commented Dec 9, 2022

The () will be required only in the enum case, right?

Sure, otherwise everything would be broken :). I've only swapped enum case constants for expressions in the parser.

@kerams
Copy link
Contributor Author

kerams commented Dec 9, 2022

All good now, I think.

@kerams
Copy link
Contributor Author

kerams commented Dec 10, 2022

I could also take this as an opportunity to adjust error ranges if you want. https://sharplab.io/#v2:DYLgZgzgNAJiDUAfALgTwA4FMAEBBbAvALABQ252i2AGodgIwAypFl2AmnQExA== should only highlight the expression range.

@T-Gro
Copy link
Member

T-Gro commented Dec 12, 2022

I could also take this as an opportunity to adjust error ranges if you want. https://sharplab.io/#v2:DYLgZgzgNAJiDUAfALgTwA4FMAEBBbAvALABQ252i2AGodgIwAypFl2AmnQExA== should only highlight the expression range.

Agree, it makes more sense to use the expression's range only.

T-Gro
T-Gro previously approved these changes Dec 12, 2022
T-Gro
T-Gro previously approved these changes Dec 13, 2022
@kerams
Copy link
Contributor Author

kerams commented Jan 5, 2023

Bump

@vzarytovskii vzarytovskii enabled auto-merge (squash) January 21, 2023 23:55
@vzarytovskii vzarytovskii merged commit 463ce71 into dotnet:main Jan 21, 2023
@kerams kerams deleted the e branch January 22, 2023 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants