Skip to content

Consider namespace during collision detection#396

Merged
KirillOsenkov merged 1 commit intogluck:masterfrom
schleupen:fix_renamed_type_already_exists
Feb 28, 2025
Merged

Consider namespace during collision detection#396
KirillOsenkov merged 1 commit intogluck:masterfrom
schleupen:fix_renamed_type_already_exists

Conversation

@vha-schleupen
Copy link
Copy Markdown
Contributor

No description provided.

@KirillOsenkov
Copy link
Copy Markdown
Collaborator

It's hard to understand the motivation without an explanation, or example and no tests. FullName should include namespace I think?

@vha-schleupen
Copy link
Copy Markdown
Contributor Author

First: We have to switch from TargetAssemblyMainModule.GetType(...) to TargetAssemblyMainModule.Types.FirstOrDefault(...), because .GetType(...) internally accesses a dictionary with cached type names. After renaming a type, however, this cache is not updated. .Type.FirstOrDefault(...) accesses the real list of types.

Secondly: There was an error in line 174, where only the name was checked, not the fullname.

@KirillOsenkov KirillOsenkov merged commit 58ae767 into gluck:master Feb 28, 2025
@KirillOsenkov
Copy link
Copy Markdown
Collaborator

I see. Thanks.

@vha-schleupen vha-schleupen deleted the fix_renamed_type_already_exists branch February 28, 2025 09:16
@KirillOsenkov
Copy link
Copy Markdown
Collaborator

@vha-schleupen this has created a regression for nested types. For two nested types in different parent types, if the name and namespace is the same, it can pick the wrong nested type.

This also needs to account the chain of declaring types and their names and namespaces.

Issue #405 is because of this.

KirillOsenkov added a commit that referenced this pull request May 26, 2025
NuGet.Protocol.dll has two types:

EnumerableAsync`1
and
LocalPackageListResource/EnumerableAsync`1

Need to take into account parent types when comparing types.

This regressed in #396

Fixes #405
@KirillOsenkov
Copy link
Copy Markdown
Collaborator

I have a fix in 7137b8c, could you please check that it looks OK.

@vha-schleupen
Copy link
Copy Markdown
Contributor Author

@KirillOsenkov I have checked it. It looks good to me. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants