[Proposal]: Breaking change warnings

[MadsTorgersen]

Breaking change warnings

• Proposed
• Prototype: Not Started
• Implementation: Not Started
• Specification: https://github.com/dotnet/csharplang/blob/main/proposals/breaking-change-warnings.md

Please see the proposal here.

LDM Discussions

https://github.com/dotnet/csharplang/blob/main/meetings/2023/LDM-2023-10-16.md#breaking-change-warnings
https://github.com/dotnet/csharplang/blob/main/meetings/2024/LDM-2024-02-07.md#breaking-change-warnings
https://github.com/dotnet/csharplang/blob/main/meetings/2024/LDM-2024-03-04.md#breaking-changes-making-field-and-value-contextual-keywords

[iam3yal]

Personally I don't like the two far extreme alternatives and I think that there shouldn't be an option to disable the warnings and the only way forward is to fix the code be it manually, through fixers or this hypothetical upgrade tool that does this automatically.

[DavidArno]

As it stands, this feature is extremely worrying as it will warn that silent breaking changes have been introduced to the language, but only if you do the right combination of update the SDK, then compile, then change the target framework. If you don't perform that compile step at the right point, there will be no warning of the change.

The limited alternatives offered ignore two obvious - and in my opinion, far superior - options:

1 - Always warn.

For the code example:

public class Entity
{
    string field;
    public string Field
    {
        get { return field; }         
        set { field = value.Trim(); } 
    }
    ...
}

if I target C# 11 or lower, I get a warning that this code will introduce a breaking change in C# 12 as described in the specification. If I target C# 12, then I now get a warning on the string field; line that this field is not used by the Field property. The breaking change is now no longer silent.

2 - Avoid new contextual keywords.

This approach is a radical one for a conservative language like C#, but it creates the cleanest break. For C# 11 or lower, I get a warning that field is being promoted to a keyword in C# 12. In C# 12, the string field; line becomes a compilation error. I have clear choices this way: rename to @field, rename completely to eg name or delete it and use the new field feature.

[jnm2]

@DavidArno "2 - Avoid new contextual keywords" doesn't catch a silent breaking change where the field was already declared as string @field; and referenced inside the accessor as either field or @field, though this break is much less likely since there would have likely been no pressure from tooling to declare using @ in the first place.

[aradalvand]

+1 Despite the possible (and likely) resistance that many people might show against this, I believe it's a direly needed policy shift, and one that's becoming increasingly relevant as more and more features are added to the language.

It's a great way (and perhaps the only way) to mitigate the bloat problem, by essentially simplifying the language and reducing the amount of gotchas and "special rules" that developers have to learn and be aware of.

[MisinformedDNA]

Should breaking changes get warnings or errors? Allowing this

public class Entity
{
    string field;
    public string Field
    {
        get { return field; }         
        set { field = value.Trim(); } 
    }
    ...
}

when field is a keyword, will be confusing for new devs on the project.

I'm thinking this should be a warning in C# 11- and an error in C# 12+. Developers can resolve the issue by renaming field (i.e. _field) or using this.field.

Prepending this. is also an easy fix for a Roslyn code fix.

[DavidArno]

@DavidArno "2 - Avoid new contextual keywords" doesn't catch a silent breaking change where the field was already declared as string @field; and referenced inside the accessor as either field or @field, though this break is much less likely since there would have likely been no pressure from tooling to declare using @ in the first place.

I would have to:

1. Refer to it as @field outside of the property and field within the property
2. Miss the C# 11 or lower warning that field becomes a breaking change in C# 12
3. There not be any warning that the @field field is now longer initialised/read when I moved to C# 12

This does feel like an extreme edge-case, but more importantly, it is far more robust than what the team has so far proposed.

[CyrusNajmabadi]

This does feel like an extreme edge-case

How much effort should be put into extreme edge cases. We def hear the feedback pretty firmly from lots of our very vocal parts of our community that too much time is spent on cases that don't actually occur in practice.

[CyrusNajmabadi]

when field is a keyword, will be confusing for new devs on the project.

I imagine people will just not write code like that. Similar to how i do not expect people to write types called var or await. If code is confusing based on how hte language works, people will change it if they don't want that confusion.

Note: following .net naming guidelines would def help here for mitigating these sorts of potential issues.

[MisinformedDNA]

when field is a keyword, will be confusing for new devs on the project.

I imagine people will just not write code like that. Similar to how i do not expect people to write types called var or await. If code is confusing based on how hte language works, people will change it if they don't want that confusion.

Note: following .net naming guidelines would def help here for mitigating these sorts of potential issues.

Totally agree. My point is that if you want a breaking change, I prefer an error over a warning that people can ignore and/or hide.

[MisinformedDNA]

How much effort should be put into extreme edge cases. We def hear the feedback pretty firmly from lots of our very vocal parts of our community that too much time is spent on cases that don't actually occur in practice.

If there is no evidence that people write code in specific, bizarre ways, I wouldn't waste too much time trying to account for theoretical scenarios. A Roslyn error for the common ones and then the rest can be covered in release notes/breaking changes.

Also, since C# versions are loosely tied to .NET target frameworks, I would include C# breaking changes with the .NET breaking changes list.

[CyrusNajmabadi]

@MisinformedDNA thanks! :) Will def be something we'll be thinking about. In the last meeting on the topic i firmly put forth this should be a strong, non-ignorable, error. But we'll have to see how it falls out when all the voices and scenarios are heard.

[HaloFour]

@CyrusNajmabadi

Note: following .net naming guidelines would def help here for mitigating these sorts of potential issues.

In what way? The naming guidelines only apply to the public surface and don't care what you name private fields or locals. I've definitely used the name field for both.

[CyrusNajmabadi]

The .net naming conventions say to name your fields with underscore. (Note: i'm not using this to say that thus we don't care about people who don't do that. Simply that following these published conventions will help put your code in the state where the issues are def avoided).

[HaloFour]

@CyrusNajmabadi

The .net naming conventions say to name your fields with underscore.

Where? All I'm seeing is that the naming guidelines don't cover internal or private members, and to not prefix field names.

Nevermind, I do see it explicitly under the C# coding conventions:

Use camel casing ("camelCasing") when naming private or internal fields, and prefix them with _.

When working with static fields that are private or internal, use the s_ prefix and for thread static use t_.

I, personally, abhor both, as much as I abhor Systems Hungarian.

To each their own, and I'm not arguing against treating field as a contextual keyword within properties for the sake of semi-auto-properties, I object to the notion that if people strictly adhere to a prescribed coding convention (esp. for aspects that don't impact the API surface) that they can expect friction from language changes.

[CyrusNajmabadi]

Specified here: https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/coding-style.md

We use _camelCase for internal and private fields and use readonly where possible. Prefix internal and private instance fields with _,

I, personally, abhor both, as much as I abhor Systems Hungarian.

That's fine. As mentioned, no one is expected to follow this. Just that if you do, you have a lower chance of running into any issues with this sort of break.

I object to the notion that if people strictly adhere to a prescribed coding convention (esp. for aspects that don't impact the API surface) that they can expect friction from language changes.

I wasn't saying that. Nor am i saying that if you don't follow these conventions that that is considered any less of an important scenario than if you do :)

[AlwaysHC]

public string Name { get; set => Name = value.Trim(); }
Why not?

[CyrusNajmabadi]

Already has existing meaning, and would break existing recursive properties.

[ufcpp]

The .net naming conventions say to name your fields with underscore.

If so, isn't it better to change the default behavior of dotnet_naming_rule as well. I always put the following in .editorconfig, but I think this should be the default.

dotnet_naming_rule.private_or_internal_field_should_be_begin_with__.severity = suggestion
dotnet_naming_rule.private_or_internal_field_should_be_begin_with__.symbols = private_or_internal_field
dotnet_naming_rule.private_or_internal_field_should_be_begin_with__.style = begin_with__

dotnet_naming_symbols.private_or_internal_field.applicable_kinds = field
dotnet_naming_symbols.private_or_internal_field.applicable_accessibilities = internal, private
dotnet_naming_symbols.private_or_internal_field.required_modifiers = 

dotnet_naming_style.begin_with__.required_prefix = _
dotnet_naming_style.begin_with__.required_suffix = 
dotnet_naming_style.begin_with__.word_separator = 
dotnet_naming_style.begin_with__.capitalization = camel_case

[lskyum]

With respect to the "field" feature, here is just a random idea I came up with.

public string Name { get; set => fieldof(Name) = value.Trim(); }

I know it's a bit verbose, but personally I think that's okay for clarity.
Also the field can be referenced from other methods in the same class.

[CyrusNajmabadi]

fieldof(Name) could have meaning today. Meaning that the above would be a breaking change for existing code. Hence why we'd need a 'breaking change strategy' like what is being discussed here :)

[yaakov-h]

@lskyum that wouldn't allow for ref usages such as with LazyInitializer.EnsureInitialized, out usages, comparing against the existing value, etc.

(and is also orthogonal to the entire discussion about allowing breaking changes as a whole, not just the field feature.)

[TehPers]

I think talking about how specific features could be changed is out of scope for this proposal, since there are already prior changes to the language that could have been cleaner if they had been breaking changes (ignoring that they could break existing code). Discards (_) come to mind with how they interact with callbacks. Also, as prior art, Rust's edition system has been used effectively for introducing new syntax and cleaning up old behavior across edition boundaries.

I think the motivation for this change is strong, the main challenge comes from how you ensure that users see the warnings before migrating to a newer language version. Tooling can help automate migrations (renaming fields and whatnot), but users who do not update their compiler until they update to a newer language version will miss those warnings. With the relative velocity at which C# releases new language versions, would it make sense to introduce the warnings in version n + 1 and make the breaking change in version n + 2 (where n is current version)? It would mean that breaking changes are delayed by a language version, but could help ensure that users see the warnings and have time to adjust their code. This of course doesn't account for users who skip version n + 1, but they'll hopefully see that their code is broken when they upgrade to version n + 2.

[huoyaoyuan]

if I target C# 11 or lower, I get a warning that this code will introduce a breaking change in C# 12 as described in the specification. If I target C# 12, then I now get a warning on the string field; line that this field is not used by the Field property. The breaking change is now no longer silent.

I'm agreed with the "always warn" approach, probably at both call site and definition. Warning of each keyword should have separated warning ID so that people can suppress them if unaffected.

It also feels similar to the warning about type name record.

[Gnamra]

@MisinformedDNA thanks! :) Will def be something we'll be thinking about. In the last meeting on the topic i firmly put forth this should be a strong, non-ignorable, error. But we'll have to see how it falls out when all the voices and scenarios are heard.

I strongly agree with this. Compiler strictness is why I love Rust and I would love to see the compiler become stricter as time moves on, especially on nullability, and I can't wait for #113 or #7016 to make their way to the language. The proposal mentions "very limited breaking changes", but I think no breaking change is too big if the result is worth it. Modern .NET is proof of that. I think this is a very important issue as it directly affects how the language will evolve, so here's my thoughts on this proposal.

• Depending on the change, warnings or a tool may lead to a bad developer experience.
  If, after upgrading to a newer version of C# I suddenly see tens, hundreds or even thousands of errors, then that's a lot of code I have to change that the customer might not see the immediate benefit of and the changes may have a big test impact which may result in the customer refusing the upgrade. This sounds like an argument against errors, but I consider it an argument for as this makes it much more visible what's in store for you and your customer if you decide to upgrade, and you will be incentivized to understand what the breaking change means to your understanding of the language. This should also be true if they're treated as warnings, but the reality is that warnings are often ignored until they cause an issue which may not happen immediately, and when they do it may be too late to revert back to an earlier version of C#. An upgrading tool may have bugs that can lead to the same situation as ignoring the warnings, or as mentioned in the proposal, developers may choose to not use it making it practically the same as a warning. As such this is an argument for using non-ignorable errors or no breaking changes.

• How will this affect the C# design team when they consider breaking changes?
  Any experienced programmer knows that iteration is key to building something great. I want it to be easy for the C# design team to make breaking changes, even large ones like promoting nullable warnings to non-ignorable errors (please?), and to enable fixing design choices that at the time were deemed good. Learning C# / .NET is a complex and time consuming task. What does SynchronizationContext do?, When to use record vs class vs struct, When should one use nullable types in c# and the list goes on. My opinion is that the design team should strive to reduce the complexity while maintaining the expressiveness of the language as much as possible, and for that I believe breaking changes will be required. As such I think that the "Don't break" alternative is not even worth considering, and I am worried that an upgrading tool may prevent some breaking changes from making it into the language if the tool itself proves too time consuming to create due to complex changes or unforeseen circumstances.

• Warnings can be ignored.
  As previously mentioned, warnings (intentionally or not) can be ignored, especially if they're drowned out in a sea of 600 other warnings. If a change is breaking then something must be done about it, allowing developers to simply ignore it and then for them to realize that something is wrong at runtime conflicts with the purpose of having a compiler that checks your code. This exact experience is the reason why I dislike dynamically typed languages, and is another reason for why I do not think warnings is a good approach.

• Is the upgrading tool a bargaining chip for breaking changes?
  The upgrade tool to me sounds like something that's primarily useful for arguing for breaking changes. Developers can justify to their customers that they just run the tool and all is ok (ignoring test impact), and the C# language design team can use it to justify why a breaking change is worth doing, but I do not think this is a good solution to depend on. Developers should still take the time to understand what the breaking change is. The tool may have bugs, developers may bypass it, the complexity of the breaking change may pose challenges when creating the tool. If the tool is not ready in time for the release, what happens to the breaking change? Is it moved to the next version of C#? Who would be responsible for maintaining this tool? Would it be one tool that knows about all breaking changes for all C# versions, or would it be multiple tools that would have to be chained together when upgrading more than one version? It introduces a lot of uncertainty and complexity that in my opinion really isn't needed.

All in all, I think the non-ignorable errors is the best choice. They send a strong message to the developer that there is something going on that they must pay attention to, and if a breaking change isn't that then I don't know what is. The warnings can be ignored (intentionally or not) leading to frustration at no fault of the C# language designers, but the end result will be developers complaining about the language likely due to their own negligence. It does not incentivize developers to learn what the breaking change means to them in the same way as an error. The upgrading tool does sound like a good idea, but I fear that this will have the same result as warnings or worse. No breaking changes will negatively affect the ability of the language to evolve and learn from past mistakes, as such I do not think this is an option worth considering at all.