dotnet / wpf

WPF is a .NET Core UI framework for building Windows desktop applications.
MIT License
7.04k stars 1.17k forks source link

WindowsBase ref-assembly is missing some type forwards #1964

Closed airbreather closed 4 years ago

airbreather commented 5 years ago

Cannot build when referencing very old WPF libraries that look for System.Collections.ObjectModel.ObservableCollection<T> in WindowsBase.dll

Actual behavior: image

Expected behavior:

Builds fine.

Minimal repro:

  1. Unzip: SampleSolution.zip
  2. Load the solution in Visual Studio 2019 (tested on Enterprise 16.3.1)
  3. Try to build it.
  4. Note that changing SampleApp's TargetFramework to net472, it builds just fine.

My exploration:

Shot in the dark, browsing the MSIL of C:\Program Files\dotnet\packs\Microsoft.WindowsDesktop.App.Ref\3.0.0\ref\netcoreapp3.0\WindowsBase.dll, it's missing these forwards that are present in C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App\3.0.0\WindowsBase.dll:

.class extern forwarder System.Collections.ObjectModel.ObservableCollection`1
{
    .assembly extern System.ObjectModel
}
.class extern forwarder System.Collections.ObjectModel.ReadOnlyObservableCollection`1
{
    .assembly extern System.ObjectModel
}
.class extern forwarder System.Collections.Specialized.INotifyCollectionChanged
{
    .assembly extern System.ObjectModel
}
.class extern forwarder System.Collections.Specialized.NotifyCollectionChangedAction
{
    .assembly extern System.ObjectModel
}
.class extern forwarder System.Collections.Specialized.NotifyCollectionChangedEventArgs
{
    .assembly extern System.ObjectModel
}
.class extern forwarder System.Collections.Specialized.NotifyCollectionChangedEventHandler
{
    .assembly extern System.ObjectModel
}
vatsan-madhavan commented 5 years ago

/cc @rladuca

Did ref-compat miss these perhaps?

rladuca commented 5 years ago

These forwards aren't in the assembly attributes files, they were missed in the ref assembly because they live in the actual code.

System\Collections\ObjectModel\ObservableCollection.cs:15:[assembly: System.Runtime.CompilerServices.TypeForwardedTo(typeof(System.Collections.ObjectModel.ObservableCollection<>))]
System\Collections\ObjectModel\ReadOnlyObservableCollection.cs:14:[assembly: System.Runtime.CompilerServices.TypeForwardedTo(typeof(System.Collections.ObjectModel.ReadOnlyObservableCollection<>))]
System\Collections\Specialized\INotifyCollectionChanged.cs:14:[assembly: System.Runtime.CompilerServices.TypeForwardedTo(typeof(System.Collections.Specialized.INotifyCollectionChanged))]
System\Collections\Specialized\NotifyCollectionChangedEventArgs.cs:13:[assembly: System.Runtime.CompilerServices.TypeForwardedTo(typeof(System.Collections.Specialized.NotifyCollectionChangedAction))]
System\Collections\Specialized\NotifyCollectionChangedEventArgs.cs:14:[assembly: System.Runtime.CompilerServices.TypeForwardedTo(typeof(System.Collections.Specialized.NotifyCollectionChangedEventArgs))]
System\Collections\Specialized\NotifyCollectionChangedEventArgs.cs:15:[assembly: System.Runtime.CompilerServices.TypeForwardedTo(typeof(System.Collections.Specialized.NotifyCollectionChangedEventHandler))]

ApiCompat did find these in .NET 4.8 compat, but I made an error in my analysis of the ForwardedFrom/To attributes surrounding them. So they were baselined.

So overall, ApiCompat works, but this was a mistake on my part and I also need to search the code base for forwards (and generally assembly attributes) that are deeper in the code.

EDIT: Removed a prior incorrect statement. I'll have to look more into the ref-compat to see what is going on there, but we should have at least caught this in 4.8 compat.

I'll assign this to me for 3.1.

rladuca commented 4 years ago

I ran a few tests and I found that other missing type forwards are indeed reported in the reference assembly to implementation assembly compat checks. However, this is not the case for the above missing forwards. I am not yet sure why this is the case, perhaps there are missing dependencies in the call to ApiCompat and dangling forwards are ignored.

My first priority is fixing the immediate issues (hoisting all forwards into the assembly attributes files), ensuring the reference assemblies match across the board, and fixing my mistake in the 4.8 compat baselines . Then I will run some tests to see if I can get ApiCompat to appropriately find these missing forwards when running reference->implementation compat. In the end I'll either have a fix or a reproduction I can file a bug on Arcade with.

airbreather commented 4 years ago

Sorry if I'm misunderstanding something, but... I noticed that #1986 is on a different milestone (5.0) than this one (3.1). Was that intentional?

rladuca commented 4 years ago

@airbreather Starting with master allows us to iterate easily and get a version we are confident in merged and then we can start the process of getting it into release/3.1 (which requires approval). This is also why the PR is not set to close this issue since multiple PRs will be opened against it.

rladuca commented 4 years ago

This has been checked into all appropriate branches (master, 3.1, 3.0).

Closing as fixed.