Fixes for primary constructors

[mmusu3]

Adds the test cases from #3611 and fixes them.

The changes to IntroduceUsingDeclarations fix missing type name qualifiers in primary constructor parameters when the parameter type is or contains a nested type. However, I'm not sure if my changes are the best way to handle it, what do you think?

[siegfriedpammer]

oh, no, I just spent the last week rewriting that part of the decompiler.

[mmusu3]

I'm sorry I was late with this. Most of it was done days ago but I was having a troublesome week.

[siegfriedpammer]

Oh, no worries. For me it was a stressful week too, I only finished my refactoring today.

[mmusu3]

Oh I forgot to mention in the main post but I still had one unsolved issue that isn't part of #3611. It is a variable naming conflict. I had trouble creating a simple case that doesn't involve pattern matching so this will have to do. It only occurs once in my full decompiled code project so it's a low priority.

Input

private class C8(object obj)
{
    public int Test()
    {
        if (obj is int i)
            return i;
        return 0;
    }
}

Output

private class C8(object obj)
{
    public int Test()
    {
        object obj = obj; // Name conflict
        if (obj is int)
            return (int)obj;
        return 0;
    }
}

[siegfriedpammer]

Can you extract the changes to IntroduceUsingDeclarations into a separate commit? Once that is done, I can start reviewing. Thanks!

[mmusu3]

Done

[siegfriedpammer]

I am working on this right now... please wait

[siegfriedpammer]

this looks suspicious... we shouldn't skip those... instead we should just make sure that the correct resolver is used when visiting...

[siegfriedpammer]

if you want to prevent a mostly harmless "double visit" you can keep it like this, but I guess the inPrimaryConstructor field is unnecessary, you can just skip if the parent is a TypeDeclaration...

[siegfriedpammer]

if we keep it, it needs a comment

[siegfriedpammer]

❯ git diff
diff --git a/ICSharpCode.Decompiler/CSharp/Transforms/IntroduceUsingDeclarations.cs b/ICSharpCode.Decompiler/CSharp/Transforms/IntroduceUsingDeclarations.cs
index a2321bf10..1acee8776 100644
--- a/ICSharpCode.Decompiler/CSharp/Transforms/IntroduceUsingDeclarations.cs
+++ b/ICSharpCode.Decompiler/CSharp/Transforms/IntroduceUsingDeclarations.cs
@@ -189,8 +189,6 @@ sealed class FullyQualifyAmbiguousTypeNamesVisitor : DepthFirstAstVisitor
                        CSharpResolver resolver;
                        TypeSystemAstBuilder astBuilder;

-                       bool inPrimaryConstructor;
-
                        public FullyQualifyAmbiguousTypeNamesVisitor(TransformContext context, UsingScope usingScope)
                        {
                                this.ignoreUsingScope = !context.Settings.UsingDeclarations;
@@ -268,11 +266,11 @@ public override void VisitTypeDeclaration(TypeDeclaration typeDeclaration)
                                        return;
                                }

+                               // special case: primary constructor parameters are resolved in the outer context
+                               // (they cannot directly refer to the type being declared)
                                if (typeDeclaration.HasPrimaryConstructor)
                                {
-                                       inPrimaryConstructor = true;
                                        typeDeclaration.PrimaryConstructorParameters.AcceptVisitor(this);
-                                       inPrimaryConstructor = false;
                                }

                                var previousResolver = resolver;
@@ -292,12 +290,6 @@ public override void VisitTypeDeclaration(TypeDeclaration typeDeclaration)
                                }
                        }

-                       public override void VisitParameterDeclaration(ParameterDeclaration parameterDeclaration)
-                       {
-                               if (inPrimaryConstructor || parameterDeclaration.Parent is not TypeDeclaration)
-                                       base.VisitParameterDeclaration(parameterDeclaration);
-                       }
-
                        public override void VisitMethodDeclaration(MethodDeclaration methodDeclaration)
                        {
                                Visit(methodDeclaration, base.VisitMethodDeclaration);

if I change your code like this, it still works... so no need to track the primary constructor separately, just visit it in a different context.

[siegfriedpammer]

Thank you for spotting this bug/missing transformation... let me know, if you want to make the changes yourself, or how we should continue thanks!

[mmusu3]

I'm trying out your different suggestions and, while removing the VisitParameterDeclaration overload does seem to work it doesn't really feel like it should. The adjusted type name gets preserved through the second visit but it seems like the kind of thing that would break if some assumptions changed in the future. I'm no ILSpy expert though.
Your other suggestion to skip visiting parameters that are children of typeDecls also doesn't work unless I misunderstand your comment?
My original implementation still seems like the neatest option even though it uses the extra field but I will go with removing the overload if that's what you prefer.

[mmusu3]

After some more thorough testing just removing the VisitParameterDeclaration overload does not work. I am getting regressions on my decompilation project that involve parameters with generic type arguments.

Edit.
Here's an example:
Input

private class C10<T>(C10<T>.DataType data)
{
    public struct DataType { public int Value { get; set; } }

    public DataType Data => data;
}

Output

// Missing type argument V
private class C10<T>(C10<>.DataType data)
{
    public struct DataType
    {
        public int Value { get; set; }
    }

    public DataType Data => data;
}

[siegfriedpammer]

that's a different bug; now fixed see 45efc73

[mmusu3]

There are also more problems from your new commit. StructDefaultConstructorsAndFieldInitializers = false doesn't seem to work anymore and XML comments are quite broken now.

Edit. Moved case to new issue #3617

[siegfriedpammer]

Please create issues for new problems, otherwise the problems might get lost in a PR comment, if they are not fixed immediately. Let's keep the discussion on this PR focused on the PR topic. Thanks a lot for your understanding.

[mmusu3]

Ok, I'll make another issue. Was there anything more you wanted from me in this one?

[siegfriedpammer]

I'm trying out your different suggestions and, while removing the VisitParameterDeclaration overload does seem to work it doesn't really feel like it should. The adjusted type name gets preserved through the second visit but it seems like the kind of thing that would break if some assumptions changed in the future. I'm no ILSpy expert though.

I fixed the underlying issue that the annotations created by the TypeSystemAstBuilder where not using the correct types. This now makes the the FullyQualifyAmbiguousTypeNamesVisitor idempotent. This can be verified by duplicating the AcceptVisitor call at the end of the Run method.

Your other suggestion to skip visiting parameters that are children of typeDecls also doesn't work unless I misunderstand your comment?

Yeah, I got it wrong. Sorry!

My original implementation still seems like the neatest option even though it uses the extra field but I will go with removing the overload if that's what you prefer.

Yes, you are right, seems like we cannot get rid of the extra state. But could you please use try-finally for the assignments and, add a comment/explanation in the VisitParameterDeclaration override?

Thank you very much!

[mmusu3]

Done. Is that change and comment what you had in mind?

[siegfriedpammer]

Thank you for your contribution!