Remove unused private members by Youssef1313 · Pull Request #61064 · dotnet/roslyn

Youssef1313 · 2022-04-30T10:49:28Z

No description provided.

cston · 2022-05-01T22:51:47Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

        }

-        [Obsolete("Use VisitCollectionElementInitializer(BoundCollectionElementInitializer node, TypeSymbol containingType, bool delayCompletionForType) instead.", true)]
-        private new void VisitCollectionElementInitializer(BoundCollectionElementInitializer node)


VisitCollectionElementInitializer

It looks like this method was added intentionally, to catch cases where a specific method should be used instead. #Closed

@cston I added it back, but I don't get what's the problem removing it. The Obsolete attribute will catch cases where this method shouldn't be used. But deleting it will also ensure that it's not used (because it doesn't exist in the first place)

I'd like to understand more what kind of issues can happen if this is removed. Thanks!

This method hides the base class method with the same signature. I believe this method definition is intended to prevent calling the base method.

cston · 2022-05-01T22:56:49Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

        }

-        [Obsolete("Use IsIncrementalAndFactoryContextMatches")]
-        private new bool IsIncremental


IsIncremental

It looks like this property was added intentionally, to catch cases where IsIncrementalAndFactoryContextMatches should be used instead. #Closed

cston · 2022-05-01T23:00:07Z

src/Compilers/CSharp/Test/Semantic/Semantics/OperatorTests.cs

 o1 OPERATOR s2, //-ObjectKIND
 o1 OPERATOR o2  //-ObjectKIND" + Postfix;

-        private const string PostfixIncrementTemplate = Prefix + @"


PostfixIncrementTemplate

Consider keeping (and perhaps comment out?) since there is a commented out reference in TestOperatorOverloadResolution(). #Closed

cston · 2022-05-01T23:02:15Z

src/Compilers/CSharp/Test/Symbol/Symbols/Metadata/PE/BaseTypeResolution.cs

            Assert.Same(varI102, varC204.Interfaces()[1]);
        }

-        private void TestBaseTypeResolutionHelper3(AssemblySymbol[] assemblies)


TestBaseTypeResolutionHelper3

Consider keeping (and commenting out?) since there is a commented out reference in Test1(). #Closed

jcouv · 2022-05-06T16:48:31Z

src/Compilers/CSharp/Test/Symbol/Symbols/Metadata/PE/BaseTypeResolution.cs

-                Assert.IsAssignableFrom<MissingMetadataTypeSymbol>(arg);
-            }
-        }
+        // private void TestBaseTypeResolutionHelper3(AssemblySymbol[] assemblies)


nit: consider commenting out with /* ... */ instead of // ... (as you did in some other tests) #Closed

jcouv · 2022-05-06T16:50:28Z

src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/ArgumentAnalysisResult.cs


-#if DEBUG        
+#if DEBUG
+#pragma warning disable IDE0051 // Remove unused private members


Consider adding an Obsolete attribute and making the "unused member" analysis recognize the Obsolete attribute (ie. shouldn't complain). #Closed

I'm not sure why this method would be obsolete? It can be used for debugging purposes right?

Sure, but we don't reference it from source. Typically we invoke it from EE.
Fine to leave as-is

If that's the case, pls consider adding a comment saying that this is invoked dynamically.

jcouv · 2022-05-06T16:58:28Z

src/Compilers/VisualBasic/Portable/BoundTree/BoundSequencePointExpression.vb

    Partial Friend Class BoundSequencePointExpression
-
-#If DEBUG Then
-        Private Sub Validate()


We should probably add HasValidate="true" to BoundSequencePointExpression in VB's BoundNodes.xml instead #Closed

Nice! I didn't know this was a thing!

src/Compilers/VisualBasic/Portable/Symbols/MissingMetadataTypeSymbol.vb

jcouv · 2022-05-06T17:11:56Z

src/Tools/ExternalAccess/FSharp/FindUsages/FSharpSourceReferenceItem.cs

    {
        private readonly Microsoft.CodeAnalysis.FindUsages.SourceReferenceItem _roslynSourceReferenceItem;

+#pragma warning disable IDE0051 // Remove unused private members


nit: How/where is this constructor used? #Closed

It's not used. But I don't know if F# can be calling it via reflection or something?

Might be. Let's keep to be safe

jcouv

Done with review pass (iteration 13)

Youssef1313 · 2022-05-27T18:01:08Z

src/Compilers/VisualBasic/Portable/BoundTree/BoundNodes.xml


    <!-- Use this in the event that the sequence point must be applied to expression.-->
-    <Node Name="BoundSequencePointExpression" Base="BoundExpression">
+    <Node Name="BoundSequencePointExpression" Base="BoundExpression" HasValidate="true">


@333fred @jcouv @cston This is what causing the CI failure. This is what I see in the dump

The CompileMethod call in the stack trace is attempting to compile this method:

roslyn/src/Compilers/Test/Utilities/VisualBasic/CompilationTestUtils.vb

Lines 285 to 297 in 9c4920a

Public Function ParseSourceXml(sources As XElement,

parseOptions As VisualBasicParseOptions,

Optional ByRef assemblyName As String = Nothing,

Optional ByRef spans As IEnumerable(Of IEnumerable(Of TextSpan)) = Nothing) As IEnumerable(Of SyntaxTree)

If sources.@name IsNot Nothing Then

assemblyName = sources.@name

End If

Dim sourcesTreesAndSpans = From f In sources.<file> Select CreateParseTreeAndSpans(f, parseOptions)

spans = From t In sourcesTreesAndSpans Select s = t.spans

Return From t In sourcesTreesAndSpans Select t.tree

End Function

What happens is:

roslyn/src/Compilers/VisualBasic/Portable/Generated/BoundNodes.xml.Generated.vb

Lines 12287 to 12291 in 9c4920a

Public Overrides Function VisitSequencePointExpression(node As BoundSequencePointExpression) As BoundNode

Dim expression As BoundExpression = DirectCast(Me.Visit(node.Expression), BoundExpression)

Dim type as TypeSymbol = Me.VisitType(node.Type)

Return node.Update(expression, type)

End Function

node.Update is expecting expression.Type and type to be the same. type is originating from here:

roslyn/src/Compilers/VisualBasic/Portable/Lowering/MethodToClassRewriter/MethodToClassRewriter.vb

Lines 97 to 103 in 9c4920a

Public NotOverridable Overrides Function VisitType(type As TypeSymbol) As TypeSymbol

If type Is Nothing Then

Return type

End If

Return type.InternalSubstituteTypeParameters(Me.TypeMap).Type

End Function

I'm unable to investigate that further :(

Neme12 · 2022-05-27T18:08:14Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.cs

            Debug.Assert(value.Type is { } && (value.Type.Equals(placeholder.Type, TypeCompareKind.AllIgnoreOptions) || value.HasErrors));
        }

+#if DEBUG


This shouldn't be needed. If it's needed, that should be fixed in the analyzer. The analyzer shouldn't fire in a certain configuration for a method if the method is conditional and doesn't apply to that configuration.

it looks like it can be removed if the callsite removed #if DEBUG too. I can make that update along with fixing the current failure (which feels a bit tricky to me :( )

If this is deemed to be an analyzer bug, I wouldn't make a further effort than what you have to work around it here, unless the change is very straightforward.

Let's just make sure we've filed the bug in the appropriate place. It would be good to add a comment linking to the bug wherever our workaround is (in this commit, it would be above the #if DEBUG here.)

it looks like it can be removed if the callsite removed #if DEBUG too. I can make that update along with fixing the current failure (which feels a bit tricky to me :( )

Yes, but that seems like a workaround too. I feel like the analyzer should understand Conditional and not mark the member as unused if it's not even compiled.

EDIT: Oh, but the method is still compiled in release, even though it doesn't need to be.

jcouv · 2022-05-27T18:13:29Z

Looks like there's a legitimate failure in Correctness_Build leg.

Youssef1313 · 2022-05-27T18:13:49Z

@jcouv Yup. I've commented my observation, but unable to investigate further

Youssef1313 · 2022-05-27T18:33:50Z

@jcouv btw, the artifacts on CI don't contain a dump. I had to run eng/test-build-correctness.cmd -configuration Release -enableDumps to get a dump.

jcouv · 2022-06-08T16:13:15Z

@Youssef1313 Any luck with the bootstrapping issue? I'll mark this PR as draft for now. Ping when ready for another look. Thanks

Youssef1313 · 2022-06-08T16:15:07Z

@jcouv Not yet. I'll revert the change causing it and open an issue for it.

Youssef1313 · 2022-06-08T17:33:06Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs

            private const int NullableContextSize = 3;

            private const int HasDeclaredRequiredMembersOffset = NullableContextOffset + NullableContextSize;
-            private const int HasDeclaredRequiredMembersSize = 2;


This is currently unused. The next time a flag is added, this will need to be added back (so that the new flag offset will be the latest flag offset + latest flag size).

Let me know if you prefer a suppression?

How about commenting that line out?
@cston Do you have a preference?

Commenting the line out seems reasonable to me.

jcouv

LGTM Thanks (iteration 19)

jcouv · 2022-06-10T15:42:04Z

There still seems to be a bootstrapping issue in the correctness leg :-/

Youssef1313 · 2022-06-10T16:42:25Z

@jcouv I'll take another look to see what's going on. Converting to draft for now as I won't be able to do that soon.

Youssef1313 · 2022-06-12T14:59:10Z

@jcouv CI is green now! 🎉

This was removed in dotnet#61064, and broke visualization of BasicBlocks. I've restored it and left a comment indicating its current use.

Remove unused private members

34c8278

Youssef1313 requested review from a team as code owners April 30, 2022 10:49

ghost added Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Apr 30, 2022

Youssef1313 added 6 commits April 30, 2022 13:19

Fix CodeGenOverridingAndHiding tests

99904a8

More small progress on unused private members

b973898

Fix remaining warnings

751759d

Address more warnings

db79fdb

Add method back with #if DEBUG

3253b1f

More unused members fixes

a1e40a7

cston reviewed May 1, 2022

View reviewed changes

Youssef1313 added 6 commits May 2, 2022 13:25

Update NullableWalker.cs

97aa344

Update LanguageParser.cs

d98cde0

Update OperatorTests.cs

14cef42

Update BaseTypeResolution.cs

da096f3

Fix warnings

b408856

Fix formatting

75031e5

jcouv reviewed May 6, 2022

View reviewed changes

src/Compilers/VisualBasic/Portable/Symbols/MissingMetadataTypeSymbol.vb Outdated Show resolved Hide resolved

jcouv reviewed May 6, 2022

View reviewed changes

jcouv self-assigned this May 6, 2022

Address review comments

398dd1d

Youssef1313 commented May 27, 2022

View reviewed changes

Neme12 reviewed May 27, 2022

View reviewed changes

jcouv marked this pull request as draft June 8, 2022 16:13

auto-merge was automatically disabled June 8, 2022 16:13
Pull request was converted to draft

Disable BoundSequencePointExpression validation

d5c2d5a

Youssef1313 mentioned this pull request Jun 8, 2022

Investigate VB assertion failure #61776

Open

Youssef1313 marked this pull request as ready for review June 8, 2022 16:23

Youssef1313 added 2 commits June 8, 2022 19:16

Merge remote-tracking branch 'upstream/main' into remove-unused

4413dac

Fix build

77c2b13

Youssef1313 commented Jun 8, 2022

View reviewed changes

jcouv approved these changes Jun 8, 2022

View reviewed changes

Youssef1313 added 2 commits June 8, 2022 19:56

Comment

4bdf673

Remove one more unused method

94c866b

Youssef1313 marked this pull request as draft June 10, 2022 16:42

Fix bad change

0ba1fe5

Youssef1313 marked this pull request as ready for review June 12, 2022 14:58

jcouv merged commit a528fe2 into dotnet:main Jun 12, 2022

ghost added this to the Next milestone Jun 12, 2022

Youssef1313 deleted the remove-unused branch June 12, 2022 18:26

RikkiGibson modified the milestones: Next, 17.3 P3 Jun 28, 2022

333fred added a commit to 333fred/roslyn that referenced this pull request Nov 8, 2025

Restore used visualizer method

96be845

This was removed in dotnet#61064, and broke visualization of BasicBlocks. I've restored it and left a comment indicating its current use.

	Public Function ParseSourceXml(sources As XElement,
	parseOptions As VisualBasicParseOptions,
	Optional ByRef assemblyName As String = Nothing,
	Optional ByRef spans As IEnumerable(Of IEnumerable(Of TextSpan)) = Nothing) As IEnumerable(Of SyntaxTree)

	If sources.@name IsNot Nothing Then
	assemblyName = sources.@name
	End If

	Dim sourcesTreesAndSpans = From f In sources.<file> Select CreateParseTreeAndSpans(f, parseOptions)
	spans = From t In sourcesTreesAndSpans Select s = t.spans
	Return From t In sourcesTreesAndSpans Select t.tree
	End Function

	Public Overrides Function VisitSequencePointExpression(node As BoundSequencePointExpression) As BoundNode
	Dim expression As BoundExpression = DirectCast(Me.Visit(node.Expression), BoundExpression)
	Dim type as TypeSymbol = Me.VisitType(node.Type)
	Return node.Update(expression, type)
	End Function

	Public NotOverridable Overrides Function VisitType(type As TypeSymbol) As TypeSymbol
	If type Is Nothing Then
	Return type
	End If

	Return type.InternalSubstituteTypeParameters(Me.TypeMap).Type
	End Function

Conversation

Youssef1313 commented Apr 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston May 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston May 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston May 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston May 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv May 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv May 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv May 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jcouv May 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Youssef1313 May 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Youssef1313 May 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Neme12 May 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv commented May 27, 2022

Uh oh!

Youssef1313 commented May 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Youssef1313 commented May 27, 2022

Uh oh!

jcouv commented Jun 8, 2022

Youssef1313 commented Apr 30, 2022 •

edited

Loading

cston May 1, 2022 •

edited

Loading

cston May 1, 2022 •

edited

Loading

cston May 1, 2022 •

edited

Loading

cston May 1, 2022 •

edited

Loading

jcouv May 6, 2022 •

edited

Loading

jcouv May 6, 2022 •

edited

Loading

jcouv May 6, 2022 •

edited

Loading

jcouv May 6, 2022 •

edited

Loading

Youssef1313 May 22, 2022 •

edited

Loading

Youssef1313 May 27, 2022 •

edited

Loading

Neme12 May 27, 2022 •

edited

Loading

Youssef1313 commented May 27, 2022 •

edited

Loading