search

dotnet / wpf

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

[release/9.0-preview4] Update dependencies from dotnet/winforms #9055

Closed dotnet-maestro[bot] closed 2 weeks ago

dotnet-maestro[bot] commented 3 weeks ago

This pull request updates the following dependencies

Coherency Updates

The following updates ensure that dependencies with a CoherentParentDependency attribute were produced in a build used as input to the parent dependency's build. See Dependency Description Format

From https://github.com/dotnet/winforms

mmitche commented 3 weeks ago

@singhashish-wpf can you take a look?

ThomasGoulet73 commented 3 weeks ago

Could this be caused by dotnet/runtime#95830 ?

mmitche commented 3 weeks ago

Yes. I beleive this requires reaction in downstream repos. /cc @carlossanlop

singhashish-wpf commented 3 weeks ago

I was trying to fix this by changing the types from ICollection to IEnumerable in the LocalizedStrings.h and related files, however there is a problem with changing those types that it doesn't fit well with the IDictionary interface where ICollection is expected. Checking more on this. Also I am unclear on the effects of this change and the potential issues which could arise due to this, we change it this way.

carlossanlop commented 3 weeks ago

@eiriktsarpalis @tannergooding ptal

tannergooding commented 3 weeks ago

I was trying to fix this by changing the types from ICollection to IEnumerable in the LocalizedStrings.h and related files, however there is a problem with changing those types that it doesn't fit well with the IDictionary interface where ICollection is expected. Checking more on this. Also I am unclear on the effects of this change and the potential issues which could arise due to this, we change it this way.

The general issue is the same as would exist if you had implemented both IDictionary<TKey, TValue> and IReadOnlyDictionary<TKey, TValue> before-hand (which is what Dictionary<TKey, TValue> itself does). -- It's notably also the same general issue that exists for IEnumerator<T> IEnumerable<T>.GetEnumerator() vs IEnumerator IEnumerable.GetEnumerator which you handle here: https://github.com/dotnet/wpf/blob/main/src/Microsoft.DotNet.Wpf/src/DirectWriteForwarder/CPP/DWriteWrapper/LocalizedStrings.h#L299-L308

C++/CLI doesn't support covariant returns and doesn't properly understand DIMs, so you need to explicitly implement the conflicting member. Simply doing something similar to the following will resolve the source break

property System::Collections::Generic::IEnumerable<CultureInfo^>^ Keys2
{
    virtual System::Collections::Generic::IEnumerable<CultureInfo^>^ get() sealed =
        System::Collections::Generic::IReadOnlyDictionary<CultureInfo^, String^>::Keys::get;
}
property System::Collections::Generic::IEnumerable<String^>^ Values2
{
    virtual System::Collections::Generic::IEnumerable<String^>^ get() sealed =
        System::Collections::Generic::IReadOnlyDictionary<CultureInfo^, String^>::Values::get;
}
singhashish-wpf commented 2 weeks ago

Thanks @tannergooding I tried adding to LocalizedString.h


            property System::Collections::Generic::IEnumerable<CultureInfo^>^ Keys2
            {
                virtual System::Collections::Generic::IEnumerable<CultureInfo^>^ get() sealed =
                    System::Collections::Generic::IReadOnlyDictionary<CultureInfo^, String^>::Keys::get;
            }
            property System::Collections::Generic::IEnumerable<String^>^ Values2
            {
                virtual System::Collections::Generic::IEnumerable<String^>^ get() sealed =
                    System::Collections::Generic::IReadOnlyDictionary<CultureInfo^, String^>::Values::get;
            }

Also added the below to LocalizedStrings.cpp

   IEnumerable<CultureInfo^>^ LocalizedStrings::Keys2::get()
    {
        return (IEnumerable<CultureInfo^>^)KeysArray;
    }

    IEnumerable<String^>^ LocalizedStrings::Values2::get()
    {
        return (IEnumerable<String^>^)ValuesArray;
    }

Currently facing multiple instances of below error:

C:\Users\singhashish\source\repos\wpf\src\Microsoft.DotNet.Wpf\src\System.Printing\CPP\src\gdiexporter\gdibitmap.cpp(151,30): error C2668: 'System::Collections::Generic::IReadOnlyCollection<System::Windows::M
edia::Color>::Count::get': ambiguous call to overloaded function [C:\Users\singhashish\source\repos\wpf\src\Microsoft.DotNet.Wpf\src\System.Printing\System.Printing.vcxproj]
C:\Users\singhashish\source\repos\wpf\src\Microsoft.DotNet.Wpf\src\System.Printing\CPP\src\gdiexporter\gdipath.cpp(574,1): error C2668: 'System::Collections::Generic::IReadOnlyCollection<System::Windows::Poin
t>::Count::get': ambiguous call to overloaded function [C:\Users\singhashish\source\repos\wpf\src\Microsoft.DotNet.Wpf\src\System.Printing\System.Printing.vcxproj]
C:\Users\singhashish\source\repos\wpf\src\Microsoft.DotNet.Wpf\src\System.Printing\CPP\src\gdiexporter\gdipath.cpp(576,1): error C2668: 'System::Collections::Generic::IReadOnlyCollection<System::Windows::Poin
t>::Count::get': ambiguous call to overloaded function [C:\Users\singhashish\source\repos\wpf\src\Microsoft.DotNet.Wpf\src\System.Printing\System.Printing.vcxproj]

Looks like I am missing something here, Any further pointers?

tannergooding commented 2 weeks ago

These can be resolved by doing something similar to count = static_cast<ICollection<Color>^>(colors)->Count; or count = static_cast<IReadOnlyCollection<Color>^>(colors)->Count;.

C++/CLI doesn't like that both int IReadOnlyCollection<T>.Count { get; } and int ICollection<T>.Count { get; } exist and wants you to explicitly indicate which you want to call by casting. C# has a tie-breaking rule to avoid such ambiguities, and while C++/CLI has gotten some improvements to handle similar cases, this one looks to not be handled yet.

jkotas commented 2 weeks ago

C++/CLI doesn't like that both int IReadOnlyCollection.Count { get; } and int ICollection.Count { get; } exist and wants you to explicitly indicate which you want to call by casting.

I do not think that this is acceptable workaround. We should revert the change that introduced the break in dotnet/runtime.

tannergooding commented 2 weeks ago

We should revert the change that introduced the break in dotnet/runtime.

I think it’s worth explicitly calling out that there’s seemingly 3 lines needing changed here, for what is very likely temporary workaround here (C++/CLI has taken tie breaking improvements several times over the past few years) and that the change is inline with how regular C++ expects such ambiguities to be resolved (this type of workaround is not unique to C++/CLI)

The alternative is backing out one of the more highly requested changes, which goes back to .NET Framework 4.5 when the new interfaces were introduced, and where we will miss the biggest opportunity to get early feedback on it from the broader ecosystem. Given the years of deliberating taken to even try it this time, backing it out may substantially hinder future efforts (either to try this ask again or to look at similar asks in the future)

Given that, I personally think it’s worth at least a little further discussion around this, the general impact, and the likelihood of a new tie breaker being added to avoid the issue by the time November rolls around

jkotas commented 2 weeks ago

Given the years of deliberating taken to even try it this time, backing it out may substantially hinder future efforts (either to try this ask again or to look at similar asks in the future)

As you know from internal discussions, we have cut corners with C/C++ validation and dismissed concerns that some people involved had about it.

The learning for future efforts like this is that cutting corners is not acceptable. It has very high chance of firing back. It is not the first time we have learned it with efforts like this. We just keep re-learning it.

jkotas commented 2 weeks ago

The offending change was reverted in dotnet/runtime: https://github.com/dotnet/runtime/pull/101645. We need to wait for the new build to flow into this PR.

singhashish-wpf commented 2 weeks ago

/azp run

azure-pipelines[bot] commented 2 weeks ago
Azure Pipelines successfully started running 1 pipeline(s).