Fix extra warning for unused local function by Youssef1313 · Pull Request #47473 · dotnet/roslyn

Youssef1313 · 2020-09-04T23:23:40Z

Youssef1313 · 2020-09-04T23:25:10Z

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

            VisitArgumentsBeforeCall(node.Arguments, node.ArgumentRefKindsOpt);

-            if (!callsAreOmitted && node.Method?.OriginalDefinition is LocalFunctionSymbol localFunc)
+            if (node.Method?.OriginalDefinition is LocalFunctionSymbol localFunc)


Since node is actually a BoundCall, then we already know the local function is used. We want to visit it regardless it's conditionally omitted or not.

I worry that this change may be unsound (allowing accessing an unassigned variable). Please add a test for:

public class C { public void M() { int i; local(); i.ToString(); [Conditional("DEBUG")] void local() { i = 1; } } }

It will be good to add this test. I believe it will produce the desired error on i.ToString() because we reset the definite assignment state after visiting the call when callsAreOmitted is true.

Oh wait. I forgot that we disallow non-static local functions from having ConditionalAttribute at all. We have specified that a local function must be static in order to be conditional.

we disallow non-static local functions from having ConditionalAttribute at all.

That saved some extra debugging 😄

@jcouv Please take another look at your convenience. I do not think there is a supported scenario involving capturing variables in a conditional local function.

Sounds good. That's great.
Can we update the test (LocalFunction_ConditionalAttributeDisallowed). We should be able to remove the #pragma warning.

src/Compilers/CSharp/Test/Semantic/FlowAnalysis/LocalFunctions.cs

Youssef1313 · 2020-09-05T00:40:09Z

CI is failing due to the following test

roslyn/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenLocalFunctionTests.cs

Lines 5513 to 5540 in 1ed3ef5

    
                   [Fact] 
        
                   public void StaticLocalFunction_ConditionalAttribute_Unreferenced() 
        
                   { 
        
                       var source = @" 
        
           using System.Diagnostics; 
        
           using System; 
        
           class C 
        
           { 
        
               static void Main() 
        
               { 
        
                   local1(); 
        
                   [Conditional(""DEBUG"")] 
        
                   static void local1() 
        
                   { 
        
                       Console.Write(""hello""); 
        
                   } 
        
               } 
        
           } 
        
           "; 
        
                       CreateCompilation(source, parseOptions: TestOptions.Regular9).VerifyDiagnostics( 
        
                           // (12,21): warning CS8321: The local function 'local1' is declared but never used 
        
                           //         static void local1() 
        
                           Diagnostic(ErrorCode.WRN_UnreferencedLocalFunction, "local1").WithArguments("local1").WithLocation(12, 21)); 
        
                       CreateCompilation(source, parseOptions: TestOptions.Regular9.WithPreprocessorSymbols("DEBUG")).VerifyDiagnostics(); 
        
                   }

Should I delete my test and update the expected for the failing test?

RikkiGibson · 2020-09-05T04:45:55Z

@Youssef1313 I went in and made the edits to the tests. Thanks for tracking down the root cause of this 😄

I feel confident we want to make the behavior change given by this PR, since it brings us into alignment with how unused local variable warnings work:

using System.Diagnostics;

int x = 42;
local(x); // comment out this call and warning CS0219 is introduced, regardless of build configuration

[Conditional("DEBUG")]
static void local(int x1) { }

RikkiGibson · 2020-09-07T19:58:53Z

@dotnet/roslyn-compiler Please review this very straightforward bug fix

jcouv

LGTM Thanks (iteration 3). Consider updating existing test for [Conditional] on non-static local function.

Youssef1313 · 2020-09-09T21:37:42Z

@jcouv The test you're referring to should fail if we removed the #pragma warning because the local function there is indeed unused. The PR removes the warning only if the method is actually used.

333fred · 2020-09-09T21:47:47Z

Thanks @Youssef1313!

Fix extra warning for unused local function

a728766

Youssef1313 requested a review from a team as a code owner September 4, 2020 23:23

Youssef1313 commented Sep 4, 2020

View reviewed changes

src/Compilers/CSharp/Test/Semantic/FlowAnalysis/LocalFunctions.cs Outdated Show resolved Hide resolved

RikkiGibson added Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Sep 5, 2020

RikkiGibson added 2 commits September 4, 2020 21:44

Update CodeGenLocalFunctionTests.cs

acd14f2

Update LocalFunctions.cs

4622a98

RikkiGibson approved these changes Sep 5, 2020

View reviewed changes

RikkiGibson requested a review from a team September 7, 2020 19:58

jcouv self-assigned this Sep 9, 2020

jcouv approved these changes Sep 9, 2020

View reviewed changes

333fred merged commit ebdda46 into dotnet:master Sep 9, 2020

ghost added this to the Next milestone Sep 9, 2020

Youssef1313 deleted the 47463 branch September 9, 2020 21:52

dibarbet modified the milestones: Next, 16.8.P4 Sep 22, 2020

Conversation

Youssef1313 commented Sep 4, 2020

Uh oh!

Youssef1313 Sep 4, 2020

Choose a reason for hiding this comment

Uh oh!

jcouv Sep 7, 2020

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Sep 7, 2020

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Sep 7, 2020 • 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 Sep 7, 2020

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Sep 9, 2020

Choose a reason for hiding this comment

Uh oh!

jcouv Sep 9, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Youssef1313 commented Sep 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RikkiGibson commented Sep 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RikkiGibson commented Sep 7, 2020

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

Youssef1313 commented Sep 9, 2020

Uh oh!

333fred commented Sep 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

RikkiGibson Sep 7, 2020 •

edited

Loading

Youssef1313 commented Sep 5, 2020 •

edited

Loading

RikkiGibson commented Sep 5, 2020 •

edited

Loading