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

Deduplicate and sort error codes #15876

Merged
merged 28 commits into from
Sep 1, 2023
Merged

Deduplicate and sort error codes #15876

merged 28 commits into from
Sep 1, 2023

Conversation

Happypig375
Copy link
Member

No description provided.

@Happypig375 Happypig375 requested a review from a team as a code owner August 27, 2023 14:29
@auduchinok
Copy link
Member

Why the numbers are changed? The tooling, nowarn, and *.fsproj files all rely on these number never changing.

@T-Gro
Copy link
Member

T-Gro commented Aug 28, 2023

This is a good attempt.

I think that changing codes of preview features is fine.
Codes for hard errors should work fine as well.

But if there is an existing warning within the changeset, that might be a breaking change due to the way codes are used in compiler switches to turn warnings on/off and warnaserror.

Was this checked?

@Happypig375
Copy link
Member Author

@auduchinok reason in title

@Happypig375
Copy link
Member Author

@T-Gro ah I didn't notice the invalid directive was a warning and not an error. I'll change the other one instead.

@vzarytovskii
Copy link
Member

I allso assume that a bunch of FSAC tests will be broken cc @baronfel @TheAngryByrd

@auduchinok
Copy link
Member

@auduchinok reason in title

Sorry, I don't see any actual reason to change error numbers there.

@Happypig375
Copy link
Member Author

@auduchinok Every code should logically map to one error or warning. There has been precedence for making error codes unique: #12428

@Happypig375
Copy link
Member Author

I'll add a script to ensure increasing error codes for good measure.

@Happypig375 Happypig375 marked this pull request as draft August 28, 2023 13:23
@Happypig375 Happypig375 marked this pull request as ready for review August 28, 2023 15:36
@auduchinok
Copy link
Member

@auduchinok Every code should logically map to one error or warning. There has been precedence for making error codes unique: #12428

I understand it, yes. I still can't see why 1503 is changed to 1003. There were no duplicates of it?

@Happypig375
Copy link
Member Author

Happypig375 commented Aug 29, 2023

@auduchinok That error is related to invoking the compiler via the command line and is written between codes 1000 and 1048. However, somehow, it uses 1503 which is far away from this range and may result in a potential collision in the future if someone decides to use the space between 1311 and 1999. A code between 1001 and 1047 would fit right in with the other messages about invoking the compiler via the command line. This is quite a niche error without search results and I don't think we would break anyone here.

Copy link
Member

@vzarytovskii vzarytovskii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, check is great too.
cc @dotnet/fsharp-team-msft and @auduchinok let me know if that looks okay to you?

@auduchinok
Copy link
Member

auduchinok commented Aug 30, 2023

@auduchinok let me know if that looks okay to you?

I really think we should never change error numbers based on a feeling that an error would be better with another number, but I agree that 1503 is indeed a rare one, so I won't continue insisting. Other error changes look like a real deduplication, and we should probably do this, even if it changes the numbers, unfortunately.

@T-Gro
Copy link
Member

T-Gro commented Aug 30, 2023

I don't thinks this should be a lot of new pain for merging anyway - whenever two new diagnostics are added at the same time, a merge conflict already happens anyway.
This new check will just ensure that the conflict resolution was a good one (no duplicates, in order).

=> good for me, approved.

Can be merged if all conversation are resolved.

@T-Gro T-Gro enabled auto-merge (squash) August 30, 2023 15:55
Copy link
Member

@psfinaki psfinaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Eugene in that I'd keep 1503 as it is.
But yeah it's not the end of the world on the other hand.

Copy link
Member

@psfinaki psfinaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Eugene in that I'd keep 1503 as it is.
But yeah it's not the end of the world on the other hand - as in, the world shouldn't explode from this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants