Skip to content

Conversation

@ds5678
Copy link
Contributor

@ds5678 ds5678 commented Sep 29, 2025

Problem

Resolves #3571

Solution

  • Any comments on the approach taken, its consistency with surrounding code, etc.
    • In AssignVariableNames, I made changes to CollectReservedVariableNames, GenerateForeachVariableName, and GenerateVariableName for consistent use between CollectAllLowerCaseTypeNames(ITypeDefinition) and the new CollectAllLowerCaseTypeNames(UsingScope).
  • Which part of this PR is most in need of attention/improvement?
    • Is there any reason CollectAllLowerCaseTypeNames(UserScope) shouldn't have used Linq? I noticed that CollectAllLowerCaseMemberNames(ITypeDefinition) avoided Linq, but it made my code more concise and readable.
  • At least one test covering the code changed

@ds5678
Copy link
Contributor Author

ds5678 commented Sep 29, 2025

The last failing test is the Discards pretty test. I'm not sure how to fix it without violating the spirit of the test:

// Source
public void DiscardedOutVsLambdaParameter()
{
	GetOut(out var _);
	MakeValue((@_ _) => 5);
}

// Decompiled
public void DiscardedOutVsLambdaParameter()
{
	GetOut(out var _);
	MakeValue((@_ _2) => 5);
}

for (int j = 0; j < 10; j++)
{
QualifierTests.i.Test();
i.Test();
Copy link
Member

Choose a reason for hiding this comment

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

hmm... doesn't this change defeat the purpose of the test?

Copy link
Contributor Author

@ds5678 ds5678 Oct 2, 2025

Choose a reason for hiding this comment

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

Are you suggesting this portion of the test be removed?

Copy link
Member

@siegfriedpammer siegfriedpammer Oct 2, 2025

Choose a reason for hiding this comment

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

I would like to keep the test, but it seems we cannot actually test the case where a local variable conflicts with a type name, because AssignVariableNames will no longer use nested type names as variable names :)

@siegfriedpammer
Copy link
Member

The issue you are fixing is actually not related to lower-case type names at all, but rather that nested types were not considered previously. This is true for both the nested type Discards.@_ and QualifierTests.i.

I don't see a way to actually make the tests still work with this change. Maybe we should just remove them?

@ds5678
Copy link
Contributor Author

ds5678 commented Oct 2, 2025

Good now?

@siegfriedpammer
Copy link
Member

Computer says yes! 😆

@siegfriedpammer siegfriedpammer merged commit 4ed7371 into icsharpcode:master Oct 2, 2025
5 checks passed
@siegfriedpammer
Copy link
Member

Thank you very much for the contribution!

@siegfriedpammer
Copy link
Member

  • Is there any reason CollectAllLowerCaseTypeNames(UserScope) shouldn't have used Linq? I noticed that CollectAllLowerCaseMemberNames(ITypeDefinition) avoided Linq, but it made my code more concise and readable.

I forgot to answer the question. Actually, I no longer remember why I wrote it without using LINQ. Not sure, if there ever was a performance problem in that part of the code.

@ds5678 ds5678 deleted the issue3571 branch October 2, 2025 19:55
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.

Variable naming doesn't find all accessible types with lower case names

2 participants