Update breaking change docs by terrajobst · Pull Request #13718 · dotnet/corefx

terrajobst · 2016-11-16T19:15:57Z

@danmosemsft @weshaggard @stephentoub @jkotas

skip ci please

Use line breaks and consistent headings so that code reviews are easier.

danmoseley · 2016-11-16T19:54:24Z

-* throwing a new/different exception type in an existing common scenario
-* An exception is no longer thrown
-* A different behavior is observed after the change for an input
 * renaming a public type, member, or parameter


renaming or removing

danmoseley · 2016-11-16T19:55:07Z

 * renaming a public type, member, or parameter
-* decreasing the range of accepted values within a given parameter
-* A new instance field is added to a type (impacts serialization)
 * changing the value of a public constant or enum member


or any other breaking change to the API surface area, such as making something sealed, abstract, or removing virtual, removing a base class or interface implementation, or changing a return type other than to a derived type

specifically, anything called out in this section of the other document, right? hopefluy we can just link to that here.

❗️ I think it's important to include binary breaking changes in this section (excluding potentially-avoidable ones like serialization issues), even if it's just a link to an explanation elsewhere.

I've added a link.

danmoseley · 2016-11-16T19:56:24Z

+can result in one of the following outcomes: 
+
+* **Accepted with quirking**. Depending on the estimated customer impact, we may
+  decide to "quirk" the behavior. That means, that the new behavior is only


strictly of course they aren't "quirks" in Core, it is done with AppContext. I see we are using this only in a few places, all in XML code.

Fair point. I'm following up with @AlexGhiondea to make sure the wording is correct.

danmoseley · 2016-11-16T19:57:42Z

+* **Accepted with quirking**. Depending on the estimated customer impact, we may
+  decide to "quirk" the behavior. That means, that the new behavior is only
+  exposed to apps that are compiled against the version of .NET Core that
+  introduced the change. In other words, apps running against a shared framework


That's not my understanding of how AppContext works in Core. It's different to Quirks on desktop. In Core, we don't know what version they compiled against. AppContext merely provides a way to disable behavior if they need to.

I'll follow-up with @AlexGhiondea

danmoseley · 2016-11-16T20:02:18Z

+* An exception is no longer thrown
+* A different behavior is observed after the change for an input
+* decreasing the range of accepted values within a given parameter
+* A new instance field is added to a type (impacts serialization)


This is only if we support cross version binary serialization right? Do we?

Yes, we have to.

danmoseley · 2016-11-16T20:05:17Z

@AlexGhiondea for quirking.

danmoseley · 2016-11-16T20:06:16Z

+## Bucket 2: Reasonable Grey Area
 *Change of behavior that customers would have reasonably depended on.*

 Examples:


Can we link to this section of the other document.

sharwell · 2016-11-16T23:14:36Z

 *Clear violation of public contract.*

 Examples:
-* throwing a new/different exception type in an existing common scenario


💭 This would still apply if the exception type and scenario is explicitly documented

Are you asking whether documentation changes anything? No, because people usually can't and thus don't test if their code works in accordance to our docs. They test whether their code works with our implementation :-)

sharwell · 2016-11-16T23:19:05Z


 Examples:
+
+* throwing a new/different exception type in an existing common scenario


💡 Some list items start with a capital letter, and others with a lowercase letter. It would be better to make them consistent.

sharwell · 2016-11-16T23:20:59Z

 * renaming a public type, member, or parameter
-* decreasing the range of accepted values within a given parameter
-* A new instance field is added to a type (impacts serialization)
 * changing the value of a public constant or enum member


❗️ I think it's important to include binary breaking changes in this section (excluding potentially-avoidable ones like serialization issues), even if it's just a link to an explanation elsewhere.

sharwell · 2016-11-16T23:22:55Z


 ## Bucket 2: Reasonable Grey Area
-*Change of behavior that customers would have reasonably depended on.*
+*[Change of behavior][behavioral-changes] that customers would have reasonably*


💡 Markdown doesn't require italics be applied on a line-by-line basis.

Got fooled by VS Code syntax highlighting. Fixed.

sharwell · 2016-11-16T23:25:48Z


 ## Bucket 1: Public Contract
-*Clear violation of public contract.*
+*Clear [violation][breaking-change] of public contract.*


💭 I'm torn on this link being applied to just 'violation'. It seems almost like it should apply either to 'public contract' or to the complete text 'violation of public contract'.

mellinoe · 2016-11-16T23:49:55Z

+  exposed to apps that are compiled against the version of .NET Core that
+  introduced the change. In other words, apps running against a shared framework
+  will not see the behavior if they were compiled against an older version of
+  .NET Core and thus wouldn't have been tested in the context of the behavioral


I don't really follow this section. Apps built to use a shared framework cannot run against any other version of the shared framework; they are locked to the version they built against. By definition, they must be explicitly re-built to target a different version. Therefore, I don't understand what a "quirk" means in this context. If we make a behavior-breaking change/fix in version 3.0, then existing 2.0 applications cannot possibly be affected by it, even if they are using a shared framework.

Am I misreading something here?

Apps built to use a shared framework cannot run against any other version of the shared framework; they are locked to the version they built against

...until we roll them forward. Fully side-by-side has never worked for any framework; we always ended up rolling apps forward and I expect .NET Core's shared framework dependencies to exhibit that behavior eventually as well.

I don't expect us to ever perform a roll-forward for self-contained apps as they carry their own copy of the framework.

mellinoe · 2016-11-17T00:18:41Z

+old behavior is "wrong", we still need to think through the implications. This
+can result in one of the following outcomes: 
+
+* **Accepted with compat switch**. Depending on the estimated customer impact,


Is this talking about .NET Core or just .NET Framework? I don't think we should ever have compat switches (a la .NET Framework) for .NET Core, especially not for future fixes we make.

We already have compat switches for .NET Core.

Could you link me where we're using them? I searched around and didn't see any. If we have some for existing code that's a bit unfortunate, but I really don't think we should have any more going forward, personally.

@AlexGhiondea would know more but check this out:

https://source.dot.net/#System.Private.CoreLib/src/System/AppContext/AppContextSwitches.cs,e2b18b7cfcf88355,references

@mellinoe why do you think we should not have compat switches? They are quite useful in various scenarios.

Currently we do not have the quirk semantics for them (i.e. Automatically enable switches based on TFM) but one can define them in the json file for your application and/or set them in code.

The link Immo added is about the quirks inside Corelib, but we also have them inside system.Xml as well.

we've made tons of changes that could have been put behind compat switches

It is what we have been in full .NET Framework. In .NET Core, we should be more data driven and only have quirks for behaviors that are proven to be actually breaking real .NET Core apps and being asked for. My 2 cents... .

If I remember right, it was an intentional decision to not quirk every single breaking change.

The idea (at the time) was that we would handle breaking changes by package versions. If we introduced a break in a version, people would either upgrade to the new version and deal with the break, or continue to use the previous version and not be impacted.

That model works great until we start talking about shared framework and the possibility of rolling forward user applications. In that case, you are talking about a very similar model to what we have on Desktop where the framework on which a user application is running on might change underneath them. And for these cases we have to have a way to prevent breaking them.

I don't think quirking every change is the way to go - we don't do that on Desktop either. Having some good telemetry as @jkotas suggests is going to make it easier to decide if a change needs a quirk or not.

shared framework and the possibility of rolling forward user applications

I guess this is where part of my confusion is coming from, because "rolling forward applications" is something that the product is explicitly designed to forbid right now, on quite a few different levels. Compat switches make a little more sense in that world, although I still don't like them as a technical solution.

I think no one is arguing that we need to quirk every single breaking change. The purpose of this document is to outline what flexibility we have and how we're thinking about the process, specifically for .NET Core. In fact, you're preaching to the choir as I was involved in quite a few (perfectly fine 😄 ) API additions that we had to back out or quirk due to the (insanely) high bar for .NET Framework. That's why I'm spreading the word that app-local deployments are the way to go.

At the same time, @weshaggard keeps reminding me (and right fully so) that the combination of success and sharing will involve a higher compat bar than we like. However, I also think it will never be as high as it is for .NET Framework. But practically speaking, compat switches/TFM-specific behaviors will eventually be the saving grace in order to not prevent progress.

Yes lets avoid quirks as long and much as possible... but there will come a day in .NET Core were we will need them mark my words :)

Immo Landwerth added 3 commits November 16, 2016 11:01

Apply Markdown conventions to breaking-changes.md

ab30810

Use line breaks and consistent headings so that code reviews are easier.

Move quirkable behavioral changes to bucket 2

3b782dc

Add clarification for contributors

68e4910

dnfclas added the cla-already-signed label Nov 16, 2016

danmoseley approved these changes Nov 16, 2016

View reviewed changes

danmoseley reviewed Nov 16, 2016

View reviewed changes

danmoseley mentioned this pull request Nov 16, 2016

Fixed string Join method dotnet/coreclr#8114

Merged

Address Dan's feedback

531e402

karelz added area-Meta documentation Documentation bug or enhancement, does not impact product or test code labels Nov 16, 2016

karelz assigned terrajobst Nov 16, 2016

sharwell reviewed Nov 16, 2016

View reviewed changes

sharwell approved these changes Nov 16, 2016

View reviewed changes

Address Sam's feedback

a208342

sharwell approved these changes Nov 16, 2016

View reviewed changes

mellinoe reviewed Nov 16, 2016

View reviewed changes

Change wording after talking to Alex

e9aefe1

terrajobst merged commit edf20a8 into dotnet:master Nov 17, 2016

terrajobst deleted the terrajobst/breaking-change-docs branch November 17, 2016 00:10

mellinoe reviewed Nov 17, 2016

View reviewed changes

karelz modified the milestone: 1.2.0 Dec 3, 2016


		Examples:

		* throwing a new/different exception type in an existing common scenario

Conversation

terrajobst commented Nov 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmoseley commented Nov 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

terrajobst Nov 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

terrajobst Nov 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

terrajobst Nov 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mellinoe Nov 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Nov 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

terrajobst Nov 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

terrajobst Nov 16, 2016 •

edited

Loading

terrajobst Nov 16, 2016 •

edited

Loading

terrajobst Nov 17, 2016 •

edited

Loading

mellinoe Nov 17, 2016 •

edited

Loading

jkotas Nov 17, 2016 •

edited

Loading

terrajobst Nov 17, 2016 •

edited

Loading