[go: up one dir, main page]

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

Fixed node name search for case-sensitive namespaces and types #1155

Conversation

gaschd
Copy link
Contributor
@gaschd gaschd commented Nov 6, 2024

Problem

#1154

Namespaces and types can have the same name with different casing:

Namespace CaseSensitive1
    Public Class Casesensitive1
        Public Class TestDummyAttribute
            Inherits Attribute
        End Class
    End Class
End Namespace

Namespace Issue1154
    <CaseSensitive1.Casesensitive1.TestDummy>
    Public Class UpperLowerCase
    End Class
End Namespace

The vbnode-to-csharpnode replacement searches node names with "StringComparison.OrdinalIgnoreCase".
Due to this way of comparison, the type name will be replaced with the namespace name. The duplicate namespace name will be removed at a later stage.

Solution

Performing the node name search with "StringComparison.Ordinal" first. If no match is found retry with "StringComparison.OrdinalIgnoreCase". IgnoreCase is needed as 100's of tests fail when just doing Ordinal comparison.

It is not optimal that the search is done twice now for a lot of declarations, just to handle this edge case, but checking for duplicates with different casing is probably even slower, as the collections are usually very short.

@gaschd gaschd changed the title Issue 1154 wrong nested types conversion Fixed node name search for case-sensitive namespaces and types Nov 6, 2024
@@ -229,7 +229,9 @@ private static TypeSyntax WithDeclarationNameCasing(TypeSyntax syntax, ITypeSymb

return syntax.ReplaceNodes(syntax.DescendantNodes().OfType<CSSyntax.IdentifierNameSyntax>(), (oldNode, _) =>
{
var originalName = originalNames.FirstOrDefault(on => string.Equals(on, oldNode.ToString(), StringComparison.OrdinalIgnoreCase));
string oldNodeStr = oldNode.ToString();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Existing issue - doesn't need to be fixed here:
It's a bit odd that this uses ToString on the nodes rather than using the identifier text directly. It's probably the same in the implementation, but it's weird this relies on it.

@GrahamTheCoder GrahamTheCoder merged commit bb5c35f into icsharpcode:master Nov 9, 2024
2 checks passed
@GrahamTheCoder
Copy link
Member

Thanks!

@gaschd gaschd deleted the issue-1154-wrong-nested-types-conversion branch November 10, 2024 01:38
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