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

Turn 3560 off by default, since it's hugely breaking #16160

Merged
merged 2 commits into from
Oct 23, 2023

Conversation

vzarytovskii
Copy link
Member

Heard it from multiple customers, it's producing thousands of warnings and breaking builds.

@vzarytovskii vzarytovskii requested a review from a team as a code owner October 23, 2023 14:10
@T-Gro T-Gro merged commit f41fe15 into dotnet:release/dev17.8 Oct 23, 2023
27 checks passed
ncave pushed a commit to ncave/fsharp that referenced this pull request Oct 25, 2023
* Turn 3560 off by default, since it's hugely breaking
@isaacabraham
Copy link
Contributor

Whilst I can see that this is a breaking change for people using warnings-as-errors etc., I also feel that this is a good warning to have, especially for newcomers to the lanugage. If this warning is off by default, I fear that it will never gain any serious adoption which would be unfortunate.

@@ -387,6 +387,7 @@ type PhasedDiagnostic with
| 3390 -> false // xmlDocBadlyFormed - off by default
| 3395 -> false // tcImplicitConversionUsedForMethodArg - off by default
| 3559 -> false // typrelNeverRefinedAwayFromTop - off by default
| 3560 -> false // tcCopyAndUpdateRecordChangesAllFields - off by default
Copy link
Contributor

@kerams kerams Oct 25, 2023

Choose a reason for hiding this comment

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

If this is the final solution (what about requiring a higher warning level?), it might be good to change how the warning is raised too -ErrorEnabledWithLanguageFeature is now misleading..

Copy link
Member Author

Choose a reason for hiding this comment

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

Not final, quickest one to not ship it.

@nbevans
Copy link

nbevans commented Nov 24, 2023

This is a defensive versioning pattern which this warning appears to outlaw:

type MyRecordType = { A : X; B : X; C : X }
with
   static member Default = { A = x; B = x; C = x }

let myRec = { MyRecordType.Default with A = foo; B = bar; C = zee }

Advantage is you can add a D to MyRecordType at a future date, give it a safe default, and all consumers do not break and even inherit that safe default.

The FSC should just optimise these, rather than treat as a warning.

@kerams
Copy link
Contributor

kerams commented Nov 24, 2023

That exact example was mentioned in fsharp/fslang-suggestions#603.

The FSC should just optimise these

I don't know what this means.

@nbevans
Copy link

nbevans commented Nov 24, 2023

That exact example was mentioned in fsharp/fslang-suggestions#603.

The FSC should just optimise these

I don't know what this means.

Similar example yes - but doesn't cover the use-case I presented.

My point on optimising rather than warning is essentially this:

If it's smart enough to spot these as warnings, it should be smart enough to emit IL that avoids the C&U.

@kerams
Copy link
Contributor

kerams commented Nov 24, 2023

There's no difference in IL - you just feed the record constructor some data. Indeed, the purpose of this warning is to highlight a construct that might lead to bugs under the right conditions (see the motivating example in the language suggestion). However, since this is now opt-in, it's up to you to decide whether you want to be warned or whether you often write code like the above and the warning would be unwelcome.

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.

7 participants