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

More descriptive error message for diagnostic 727 #12832

Merged
merged 11 commits into from
Sep 29, 2022

Conversation

baronfel
Copy link
Member

@baronfel baronfel commented Mar 11, 2022

Will fill in later, my local build isn't working so I'm going to piggyback onto MS's CI :D

The gist is that this error is garbage:
image

We know the expected field names and the positions provided, so we can estimate names not provided and emit them here. The message can be changed to let the user know this is a suggestion, because DU cases can be added at any point. I'd argue that it's most likely to add them at the end, though.

The new error message is something like

This union case expects 3 arguments in tupled form, but was given 2. The missing field arguments may be any of:
  <field1>
  <field2>
  ...
  <fieldN>

because that at least gives the user a clue what kinds of fields/data they can expect (without having to rely on tooltips/go-to-definition for the DU)

Open questions:

  • layout of the message itself - right now indentation isn't correct
  • any more intelligence that should be done to try to match patterns provided by the user with the intended DU fields, instead of a basic positional approach that I've looked at for 2 seconds

@baronfel baronfel force-pushed the better-du-tuple-destructure-error-message branch from 7523278 to fa01817 Compare March 11, 2022 15:33
@smoothdeveloper
Copy link
Contributor

This is amazing, and is related to #1103 if we are going to adjust the error message as well.

@T-Gro
Copy link
Member

T-Gro commented Sep 23, 2022

@baronfel , can I help with finalizing this?

To be honest, I do not think this should render "Item2" in the message if the argument is not named - would rather fallback to the type instead.

Having said that, I would be happy to pick this up from you if you agree.

@baronfel
Copy link
Member Author

Absolutely! To be honest I forgot that I submitted this PR, and I'd love someone to pick up the torch.

@T-Gro T-Gro self-assigned this Sep 23, 2022
Preventing display of generated Item2,.. names for DU field names
Displaying type of field as part of the error message.
@T-Gro T-Gro marked this pull request as ready for review September 26, 2022 11:52
@T-Gro T-Gro requested a review from nojaf September 26, 2022 12:42
@T-Gro
Copy link
Member

T-Gro commented Sep 26, 2022

Current look & feel:

image

@vzarytovskii
Copy link
Member

Current look & feel:

image

I feel like this comma placement is awkward. Do we place it in the beginning of the line somewhere else?

@T-Gro
Copy link
Member

T-Gro commented Sep 26, 2022

Checked, there are places which use Newline + tab. Newline + comma is not really used, so I am dropping it in favour of tab.

See here:
image

@T-Gro T-Gro requested review from vzarytovskii and nojaf and removed request for nojaf September 26, 2022 16:39
@T-Gro T-Gro requested review from vzarytovskii and removed request for nojaf September 27, 2022 12:57
@T-Gro T-Gro closed this Sep 29, 2022
@T-Gro T-Gro reopened this Sep 29, 2022
@T-Gro T-Gro merged commit 108ca7d into dotnet:main Sep 29, 2022
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.

5 participants