Skip to content

Fix bug in XamlTypeInvoker.GetAddMethod if there is no such match#211

Merged
dipeshmsft merged 2 commits into
dotnet:mainfrom
hughbe:xti
Apr 28, 2023
Merged

Fix bug in XamlTypeInvoker.GetAddMethod if there is no such match#211
dipeshmsft merged 2 commits into
dotnet:mainfrom
hughbe:xti

Conversation

@hughbe
Copy link
Copy Markdown
Contributor

@hughbe hughbe commented Dec 26, 2018

Fix 1: Use Dictionary.TryAdd instead of Add to prevent duplicates

Example code:

[Fact]
public void GetAddMethod_NoSuchContentType_ReturnsNull()
{
    var invoker = new XamlTypeInvoker(new XamlType(typeof(List<int>), new XamlSchemaContext()));
    var type = new XamlType(typeof(string), new XamlSchemaContext());
    Assert.Null(invoker.GetAddMethod(type));
};

Use Dictionary.TryAdd to avoid throwing if there is more than one value. I opted to change this to TryAdd rather than removing addMethods.Add(_xamlType.ItemType, _xamlType.AddMethod);, as firstly a user can override LookupContentWrappers so removing this value could be breaking, and secondly because ContentWrappers could contain duplicate types which could throw anyway previously

Expected: we return null
Actual: we throw an ArgumentException for duplicate value in a dictionary

Fixes #210

Fix 2: Bail out of GetEnumeratorMethod if the type invoker is unknown

[Fact]
public void GetEnumeratorMethod_Unknown_ReturnsNull()
{
    XamlTypeInvoker invoker = XamlTypeInvoker.UnknownInvoker;
    Assert.Null(invoker.GetEnumeratorMethod());
}

Expected: returns null, matching behaviour with GetAddMethod()
Actual: throws NullReferenceException

Fixes #216

Comment thread src/Microsoft.DotNet.Wpf/src/System.Xaml/System/Xaml/Schema/XamlTypeInvoker.cs Outdated
Comment thread src/Microsoft.DotNet.Wpf/src/System.Xaml/System/Xaml/Schema/XamlTypeInvoker.cs Outdated
Comment thread src/Microsoft.DotNet.Wpf/src/System.Xaml/System/Xaml/Schema/XamlTypeInvoker.cs Outdated
Comment thread src/Microsoft.DotNet.Wpf/src/System.Xaml/System/Xaml/Schema/XamlTypeInvoker.cs Outdated
@stevenbrix stevenbrix added the * NO MERGE * metadata: The PR is not ready for merge yet (see discussion for detailed reasons) label Apr 4, 2019
@stevenbrix stevenbrix added this to the 3.0 milestone Apr 4, 2019
@stevenbrix
Copy link
Copy Markdown
Contributor

Thanks for this PR @hughbe! I also really appreciate you breaking up the commits the way you did so that we can see the bug fix vs. general code cleanup. I'm sorry that we've just left this sitting here, but there are few reasons why we are a bit hesitant on accepting these sorts of changes:

  1. There are a lot of righteous syntax fixes here. While we really appreciate those, the entire WPF code base needs these improvements. In general, we want to adopt a workflow where these types of improvements are done via an automated tool of some sort, and ideally ones that use roslyn analyzers + codefixers to make these changes. The PR should then include how the tool was run. All your changes look great, so I'm not suggesting anything different be done here, but it's just an FYI. Many of these improvements can be subjective, so we'd rather have discussions on which analyzers+codefixers we are using on our codebase, rather then have developers go through the work and send out PR's, only to have it be rejected.
  2. As I mentioned on one of your other PR's, we are working towards getting a more modern test infrastructure out there that new test cases like this could be added to. These PR's are great candidates to help us onboard that work once we are ready.

@rladuca rladuca modified the milestones: 3.0, Future May 20, 2019
@grubioe grubioe added the PR metadata: Label to tag PRs, to facilitate with triage label May 28, 2019
Base automatically changed from master to main March 17, 2021 17:38
@ryalanms ryalanms requested a review from a team as a code owner March 17, 2021 17:38
@pchaurasia14 pchaurasia14 added the Community Contribution A label for all community Contributions label Jul 20, 2022
@ghost ghost assigned hughbe Jul 20, 2022
@dipeshmsft dipeshmsft added Queue for test pass and removed * NO MERGE * metadata: The PR is not ready for merge yet (see discussion for detailed reasons) labels Apr 17, 2023
@dipeshmsft
Copy link
Copy Markdown
Member

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@hughbe
Copy link
Copy Markdown
Contributor Author

hughbe commented Apr 27, 2023

I might need to rebase this PR

@dipeshmsft
Copy link
Copy Markdown
Member

@hughbe, let's wait for the CI build to finish.

@dipeshmsft
Copy link
Copy Markdown
Member

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@dipeshmsft dipeshmsft merged commit 2d0a39b into dotnet:main Apr 28, 2023
@hughbe hughbe deleted the xti branch April 28, 2023 18:47
@ghost ghost locked as resolved and limited conversation to collaborators May 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Community Contribution A label for all community Contributions PR metadata: Label to tag PRs, to facilitate with triage

Projects

None yet

6 participants