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

.NET Core 3.1 WindowsDesktop application can fail when rolled forward to .NET 5 preview #2921

Closed
DustinCampbell opened this issue Feb 27, 2020 · 8 comments · Fixed by #2923
Closed
Assignees
Labels
🪲 bug Product bug (most likely)

Comments

@DustinCampbell
Copy link
Member

DustinCampbell commented Feb 27, 2020

  • .NET Core Version:
    .NET Core 3.x -> .NET 5

  • Have you experienced this same bug with .NET Framework?:
    No

Problem description:

The System.Windows.Forms.Design.Editors.dll assembly has been removed in .NET 5, which can cause applications built with .NET Core 3.x to fail if they used a type from that assembly and are permitted to roll forward on major versions.

Expected behavior:

The application should not fail. Instead, a System.WinForms.Forms.Design.Editors.dll should be included that contains type forwards for all of the types that were moved out of that assembly. In this way, WinForms on .NET 5 remains binary compatible.

Minimal repro:

  1. Create a .NET Core 3 application.
  2. Add code that references a public type from System.Windows.Forms.Design.Editors.dll, e.g. AnchorEditor.
  3. Build it
  4. Run it
  5. Copy the app to a VM with only .NET5 SDK installed (or remove .NET Core 3.x SDK form your workstation)
  6. Modify a <app name>.runtimeconfig.json with the following content (update the .NET5 SDK version as necessary):
    {
      "runtimeOptions": {
    	"tfm": "netcoreapp3.0",
    	"rollForward": "LatestMajor",
    	"framework": {
    	  "name": "Microsoft.WindowsDesktop.App",
    	  "version": "5.0.0-preview.2.20119.5"
    	}
      }
    }
  7. Run the application and ensure that the code that used the type from System.Windows.Forms.Design.Editors.dll executes.

[EDIT] updated the repro steps

@merriemcgaw merriemcgaw added this to the 5.0 milestone Feb 27, 2020
@merriemcgaw merriemcgaw added the 🪲 bug Product bug (most likely) label Feb 27, 2020
RussKie added a commit to RussKie/winforms that referenced this issue Feb 28, 2020
As part of consolidation initiative (refer to dotnet#2781) we have merged
System.Windows.Forms.Design.Editors.dll into System.Windows.Forms.Design.dll.

However doing so we broke roll-forward upgrades for any app depending on
types delcared in System.Windows.Forms.Design.Editors.dll.

Create a facade assembly that forwards types from original assembly.

Resolves dotnet#2921.
@ghost ghost added the 🚧 work in progress Work that is current in progress label Feb 28, 2020
@weltkante
Copy link
Contributor

weltkante commented Feb 28, 2020

@RussKie @merriemcgaw just FYI if you intend to preserve binary compatibility between 3.x and 5.0 you cannot add proper generic collection support (#2644). Return types of GetEnumerator have to change in order for foreach to pick it up; this will break applications which roll forward from 3.x (just tested). Since loops over collections are probably going to be pretty common you can't treat it as an exceptional case, it'll probably affect every nontrivial application rolling forward from 3.x

IMHO requiring a recompile for 5.0 is not entirely unreasonable and its probably one of the better chances to modernize winforms, it'll probably only get harder from there on.

So ultimately you'll have to make a strategic decision here what your goal for the 3.x/5.0 transition is, and the sooner its done and communicated the better.

@weltkante
Copy link
Contributor

#2925 has more binary incompatibility

@AdamYoblick
Copy link
Member

Trying this repro and testing Igor's fix shortly.

@RussKie
Copy link
Member

RussKie commented Mar 2, 2020

@weltkante thank you, but you may be misinterpreting the intent of this work.

Whilst this bug is may affect our users, and it may not be totally unreasonable to ask to recompile, it is not the main driving factor here. The bug is currently blocking the VS Designer from rendering previews for different versions of .NET Core/.NET (due to a number of API moved from one assembly to another), and we don't expect to build a version of the Designer for each .NET version.

Other API proposals likely won't have a significant effect on the Designer, as those do not touch type editors and type converters.

@weltkante
Copy link
Contributor

weltkante commented Mar 2, 2020

@RussKie I'm just saying that if #2644 is implemented the work done here doesn't matter because roll-forward will fail anyways as soon as anyone has an foreach loop (which is pretty likely). Giving people the false impression that roll-forward will work in "some cases" may be counterproductive because ensuring that nobody has an foreach loop over a WinForms collection will be pretty much impossible (its enough if a single library has such a loop). Personally I'd prefer a clear error message that you need to recompile (or dropping #2644 if the impact is considered too large). In either way I wanted to make sure you are aware of the consequences the not yet merged PR would have.

we don't expect to build a version of the Designer for each .NET version. Other API proposals likely won't have a significant effect on the Designer, as those do not touch type editors and type converters.

You'll have to make sure not using foreach loops over WinForms collections in the designer then (probably can pass the collection through a helper method so the foreach doesn't see the specific class you are iterating over)

@DustinCampbell
Copy link
Member Author

DustinCampbell commented Mar 3, 2020

@weltkante: Why would roll forward break with #2644 for foreach loops? I would expect that IEnumerable implementation is not being removed from types and binary compatibility would be preserved.

@weltkante
Copy link
Contributor

weltkante commented Mar 3, 2020

The interface doesn't matter, Roslyn doesn't look at the interface, it looks at the return type of public GetEnumerator methods so those need to change to return the generic IEnumerator<> otherwise foreach won't pick it up. If the return type of the method changes you get a MethodNotFound exception when you compiled against the old GetEnumerator which returned non-generic IEnumerator.

You can implement generic collections without changing the return type (for LINQ support) but then you won't get foreach (var control in container.Controls) type inference and also won't get proper nullability annotations, so you'll have to continuously work around nullability complaints when using WinForms.

@DustinCampbell
Copy link
Member Author

@weltkante: I suppose that would result in a break if my application was previously compiled against the public GetEnumerator() method. For reason, I would argue very strongly against taking a change like that. Forward compatibility is extremely important. Introducing a break like this would break any third-party library compiled against netcoreapp3.1.

I'd be very interested in a proposal that improves productivity with the existing APIs without breaking the entire ecosystem to do.

RussKie added a commit that referenced this issue Mar 3, 2020
As part of consolidation initiative (refer to #2781) we have merged
System.Windows.Forms.Design.Editors.dll into System.Windows.Forms.Design.dll.

However doing so we broke roll-forward upgrades for any app depending on
types delcared in System.Windows.Forms.Design.Editors.dll.

Create a facade assembly that forwards types from original assembly.

Resolves #2921.
@ghost ghost removed the 🚧 work in progress Work that is current in progress label Mar 3, 2020
@RussKie RussKie removed this from the 5.0 milestone Mar 3, 2020
M-Lipin pushed a commit to M-Lipin/winforms that referenced this issue Mar 23, 2020
As part of consolidation initiative (refer to dotnet#2781) we have merged
System.Windows.Forms.Design.Editors.dll into System.Windows.Forms.Design.dll.

However doing so we broke roll-forward upgrades for any app depending on
types delcared in System.Windows.Forms.Design.Editors.dll.

Create a facade assembly that forwards types from original assembly.

Resolves dotnet#2921.
@ghost ghost locked as resolved and limited conversation to collaborators Feb 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🪲 bug Product bug (most likely)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants