Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unused binding warning isn't reported for recursive binding in a type #13849

Open
auduchinok opened this issue Sep 6, 2022 · 14 comments
Open
Labels
Area-Compiler-Checking Type checking, attributes and all aspects of logic checking Bug good first issue help wanted Impact-Medium (Internal MS Team use only) Describes an issue with moderate impact on existing code.
Milestone

Comments

@auduchinok
Copy link
Member

Consider the following type declaration:

type T() =
    let rec f x = ()
    and g x = ()

    do g ()

Earlier FCS versions would report f as unused with --warnon:1182 flag present, but the current main doesn't report it anymore.

@psfinaki Could it be somehow related to #13429?

@auduchinok auduchinok added the Bug label Sep 6, 2022
@psfinaki
Copy link
Member

psfinaki commented Sep 8, 2022

@auduchinok not that I can think of a direct connection - but cannot say for sure since CheckExpressions.fs is a very interconnected thing and everything can happen there. Or maybe @dsyme you have insights on that?

Regardless, this is a regression, not good - @vzarytovskii FYI.

@vzarytovskii
Copy link
Member

vzarytovskii commented Sep 8, 2022

It is likely related to the way we check it (i.e. we don't emit warning if member/binding has CompilerGeneratedAttribute)
Here:

let reportIfUnused() =
if not v.HasBeenReferenced && not v.IsCompiledAsTopLevel && not (v.DisplayName.StartsWithOrdinal("_")) && not v.IsCompilerGenerated then
warning (Error(FSComp.SR.chkUnusedValue(v.DisplayName), v.Range))

And here:
if not env.external &&
not alreadyDone &&
cenv.reportErrors &&
not v.HasBeenReferenced &&
not v.IsCompiledAsTopLevel &&
not (v.DisplayName.StartsWithOrdinal("_")) &&
not v.IsCompilerGenerated then
if v.IsCtorThisVal then
warning (Error(FSComp.SR.chkUnusedThisVariable v.DisplayName, v.Range))
else
warning (Error(FSComp.SR.chkUnusedValue v.DisplayName, v.Range))

Right now, we emit the following IL (roughly):

.method assembly hidebysig instance void f<a> (!!a x) cil managed 
{
    .custom instance void [System.Runtime]System.Runtime.CompilerServices.CompilerGeneratedAttribute::.ctor() = (01 00 00 00)
    .maxstack 8
    IL_0000: ret
} // end of method T::f

.method assembly hidebysig instance void g<a> (!!a x) cil managed 
{
    .custom instance void [System.Runtime]System.Runtime.CompilerServices.CompilerGeneratedAttribute::.ctor() = (01 00 00 00
    .maxstack 8
    IL_0000: ret
} // end of method T::g

We've made some changes to codegen in the following PRs: #13542 and #13494

Update: it seems those only touch fields and properties, but not methods.
@0101 I know you looked into compiler-generated attribute, do you have any insights into this?

@dsyme
Copy link
Contributor

dsyme commented Sep 8, 2022

Even just this is a repro:

type T() =
    let f _ = ()

@auduchinok
Copy link
Member Author

@dsyme Ah, interesting, indeed. In all other tests we have a simple value, not a function, and the only function in these tests happened to be recursive. I should've tried to find a smaller repro (and perhaps we need more tests 🙂). Thanks for pointing it out!

@0101
Copy link
Contributor

0101 commented Sep 8, 2022

@vzarytovskii strange, the TypedTree.Val for f somehow gets the isCompGen flag. Not sure how.

@0101
Copy link
Contributor

0101 commented Sep 8, 2022

This seems to be where it happens.

image

@dsyme
Copy link
Contributor

dsyme commented Sep 8, 2022

@0101 I think it is these lines:

                // NOTE: putting isCompilerGenerated=true here is strange.  The method is not public, nor is
                // it a "member" in the F# sense, but the F# spec says it is generated and it is reasonable to reflect on it.
                let memberValScheme = ValScheme(id, prelimTyschemeG, Some valReprInfo, None, Some memberInfo, false, ValInline.Never, NormalVal, None, true (* isCompilerGenerated *), true (* isIncrClass *), false, false)

@dsyme
Copy link
Contributor

dsyme commented Sep 8, 2022

I added this comment here, though didn't change the setting at that point:

https://github.com/dotnet/fsharp/pull/12432/files#diff-1166ef31585c9572a1ef181219c1bf3ee6cb3a444d787f0179c06cda56e96439R979-R981

@dsyme
Copy link
Contributor

dsyme commented Sep 8, 2022

Note that in the checking of incremental classes, two Val are in play in IncrementalClasses.fs:

  • We initially create a non-member value f and do most checking with respect to that.
  • After the class is fully checked we then later decide on the representation of each local let as either
    • a field
    • a constructor-local
    • a method
      If it's a method then we create a new member value f, mark it compiler-generated, and then substitute through the implementation code.

Most checking is done in TcCheck* and is done with respect to the "non-member" values which are not marked compiler-generated, however the "PostInferenceChecks" are done after the representations have been decided and so with respect to the "member" values. This is the underlying problem - the PostInferenceChecks are seeing too much of the internal compilation/representation process. We've talked about moving the IncrementalClasses representation-process into a later compiler phase - after PostInferenceChecks - but this is a larger bit of work.

I guess the fact that this is a representation-process is the reason we decided long ago to mark these as compiler-generated. Hwoever I don't know why any of this has changed recently - maybe we added the IsCompilerGenerated filter in PostInferenceChecks? @psfinaki did make a change to make the non-member vals visible to the the language service, but I don't think that should be affecting this.

Anyway, marking the "member values" as non-compiler-generated is probably OK in principle. However one problem is that there can be duplicates:

type C() =
    let f() = 1
    let f() = 2

This is allowed in F# - and the member-vals for the two f get different names, decided as the representations are chosen - in this case the member-val for the second member f is given name f@3. This is the TypedTree DisplayName/LogicalName of the member val for all post-checking - though we don't ever expect these to be reported in diagnostics.

This means a simple fix of "mark the value as non-compiler-generated" will lead to diagnostics like f@3 is unused in the second case above. It's not the end of the world if we show some compiler-generated name, but it's not "right" and it's odd that something with a compiler-generated logical name would not get marked IsCompilerGenerated - and maybe other problems will happen because of this

So maybe what we really want is two non-compiler-generated member-vals f both with the DisplayName f but with different LogicalName. There's currently no real way in the TypedTree for a Val to have a display name that is different to the logical name (except for op_..., properties etc.) but there's no real reason we can't add it. We would add it by a new optional field val_display_name_core_mangled here

mutable val_compiled_name: string option
and using it here and here and here.

That might work out I think.

@savi2w
Copy link
Contributor

savi2w commented Sep 11, 2022

We didn't emit the warning because of one flag

  • Val.IsCompilerGenerated: bool as @vzarytovskii said and → seems IsCompilerGenerated is already false
  • Val.IsCompiledAsTopLevel: bool being true here

let reportIfUnused() =
if not v.HasBeenReferenced && not v.IsCompiledAsTopLevel && not (v.DisplayName.StartsWithOrdinal("_")) && not v.IsCompilerGenerated then

After reading some code involving this attribute I think the reportIfUnused function can be simplified this way

diff --git a/src/Compiler/Checking/CheckIncrementalClasses.fs b/src/Compiler/Checking/CheckIncrementalClasses.fs
index c58414dfb..c4ae53cca 100644
--- a/src/Compiler/Checking/CheckIncrementalClasses.fs
+++ b/src/Compiler/Checking/CheckIncrementalClasses.fs
@@ -289,7 +289,7 @@ type IncrClassReprInfo =
             nm, takenFieldNames.Add nm
                  
         let reportIfUnused() = 
-            if not v.HasBeenReferenced && not v.IsCompiledAsTopLevel && not (v.DisplayName.StartsWithOrdinal("_")) && not v.IsCompilerGenerated then 
+            if not v.HasBeenReferenced && not (v.DisplayName.StartsWithOrdinal("_")) && not v.IsCompilerGenerated then 
                 warning (Error(FSComp.SR.chkUnusedValue(v.DisplayName), v.Range))
 
         let repr =

What do you guys think?

@auduchinok
Copy link
Member Author

It seems that removing v.IsCompiledAsTopLevel will enable it for top-level functions in modules leading to lots of false positive warnings?

@auduchinok
Copy link
Member Author

auduchinok commented Sep 13, 2022

Another difference is now FSharpSymbol produced for such a function returns different things:

  • mfv.DeclaringEntity returns the containing type, previously it didn't
  • mfv.IsModuleValueOrMember is true (I think it has been false previously)

@dsyme
Copy link
Contributor

dsyme commented Sep 23, 2022

Another difference is now FSharpSymbol produced for such a function returns different things:

Where is this FSharpSymbol acquired from?

@auduchinok
Copy link
Member Author

Where is this FSharpSymbol acquired from?

From results of FSharpCheckFileResults.GetAllUsesOfAllSymbolsInFile.

@T-Gro T-Gro added Impact-Medium (Internal MS Team use only) Describes an issue with moderate impact on existing code. Area-Compiler-Checking Type checking, attributes and all aspects of logic checking and removed Needs-Triage labels Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compiler-Checking Type checking, attributes and all aspects of logic checking Bug good first issue help wanted Impact-Medium (Internal MS Team use only) Describes an issue with moderate impact on existing code.
Projects
Status: New
Development

No branches or pull requests

7 participants