Fix unspeakable main name

[Youssef1313]

Related to #4253 (keeping the issue open to track using the compiler API)

[mavasani]

As commented. Just upgrading to this constant API does not warrant bumping up the code analysis version for the entire repo.

[mavasani]

We can just change this to "<Main>$"
https://github.com/dotnet/roslyn/blob/cf55f3a58e47298426fa971d3bd9d8857c746c65/src/Compilers/Core/Portable/Symbols/WellKnownMemberNames.cs#L351

[Youssef1313]

@mavasani When the code in tests is compiled, it's compiled against an older compiler version that produces $Main instead of <Main>$. I wanted to remove the old name from the extension method, so that required a newer version of the compiler. But as this is causing problems, I'll revert.

[Youssef1313]

Do you want to keep the issue open to track using the compiler API when it's good time to update the version?

[mavasani]

If you need new bits only for test you can bump up the below:

roslyn-analyzers/eng/Versions.props

Line 28 in 193a3b6

<MicrosoftCodeAnalysisForShippedApisVersion>3.7.0</MicrosoftCodeAnalysisForShippedApisVersion>

We should probably rename that property name to suggest that is for tests.

[mavasani]

Do you want to keep the issue open to track using the compiler API when it's good time to update the version?

That sounds reasonable. Thanks.

[mavasani]

This seems to be just fine.

[mavasani]

Seems like this needs a single char deletion fix :-)

[codecov]

Codecov Report

Merging #4256 into master will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #4256      +/-   ##
==========================================
- Coverage   95.80%   95.80%   -0.01%     
==========================================
  Files        1170     1168       -2     
  Lines      264526   264635     +109     
  Branches    15962    15964       +2     
==========================================
+ Hits       253428   253525      +97     
- Misses       9085     9096      +11     
- Partials     2013     2014       +1     

[mavasani]

Please retain the old logic as "$Main" was used in older compiler bits which is supported by the analyzer package. I think you should only fix the typo here.

[Youssef1313]

I thought that's not important as the compiler version has this isn't a stable release. I'll revert the last 2 commits.

[mavasani]

as the compiler version has this isn't a stable release

Ah, if you are sure it wasn't a stable release, then it would be fine to remove those cases. By the way, there are test failures.

[Youssef1313]

Ah, if you are sure it wasn't a stable release, then it would be fine to remove those cases

@mavasani I think it was shipped only in a preview version of Visual Studio. But might be better to confirm with the compiler team. Not sure who may be able help.

[Youssef1313]

@mavasani The current test failures seems unrelated to top-level statements. Can you have a look?

For example:

roslyn-analyzers/src/NetAnalyzers/UnitTests/Microsoft.CodeQuality.Analyzers/ApiDesignGuidelines/ImplementIDisposableCorrectlyTests.cs

Lines 745 to 798 in a23fde8

	public async Task CSharp_CA1063_FinalizeOverride_Diagnostic_DoubleFinalizeOverride_InvokesDisposeBool()
	{
	await VerifyCS.VerifyAnalyzerAsync(@"
	using System;
	public class A : IDisposable
	{
	public void Dispose()
	{
	Dispose(true);
	GC.SuppressFinalize(this);
	}
	~A()
	{
	Dispose(false);
	}
	protected virtual void Dispose(bool disposing)
	{
	}
	}
	public class B : A
	{
	protected override void Dispose(bool disposing)
	{
	base.Dispose(disposing);
	}
	~B()
	{
	Dispose(false);
	}
	}
	public class C : B
	{
	protected override void Dispose(bool disposing)
	{
	base.Dispose(disposing);
	}
	~C()
	{
	Dispose(false);
	}
	}
	",
	// Test0.cs(22,14): warning CA1063: Remove the finalizer from type 'B', override Dispose(bool disposing), and put the finalization logic in the code path where 'disposing' is false. Otherwise, it might lead to duplicate Dispose invocations as the Base type 'A' also provides a finalizer.
	GetCA1063CSharpFinalizeOverrideResultAt(22, 14, "B", "A"),
	// Test0.cs(35,14): warning CA1063: Remove the finalizer from type 'C', override Dispose(bool disposing), and put the finalization logic in the code path where 'disposing' is false. Otherwise, it might lead to duplicate Dispose invocations as the Base type 'B' also provides a finalizer.
	GetCA1063CSharpFinalizeOverrideResultAt(35, 14, "C", "B"));
	}

This test doesn't produce any diagnostics, while 2 are expected. The number of failing tests is huge.

[Evangelink]

@Youssef1313 I am having some trouble understanding the link between the name of the PR and its content. What am I missing?

[Youssef1313]

@Youssef1313 I am having some trouble understanding the link between the name of the PR and its content. What am I missing?

Did you mean the "unspeakable" part of the title? That's what the compiler team calls these generated members names.

[Evangelink]

@Youssef1313 I am having some trouble understanding the link between the name of the PR and its content. What am I missing?

Did you mean the "unspeakable" part of the title? That's what the compiler team calls these generated members names.

Oh I see, thanks :)

[Youssef1313]

Given these test failures here (whose reason is known for me - I may investigate later), I won't update the version to use the compiler API and just fix the typo.