Warn when obj is inferred

[Smaug123]

Fixes fsharp/fslang-suggestions#696 .

Criteria I had at the start for the definition of done:

• needs to be turned off when null is used explicitly, on the grounds that null is pretty clearly an obj. Don't know how I could do that; sounds like one for a followup PR instead. I've got tests documenting that this is currently not done.
• ditto but for typedefof ditto, there are tests documenting that this is not done
• many more tests, positive and negative, including tests that units of measure are ignored by this feature
• to be got to the point of passing the CI build when it's used to compile the compiler; currently it's hitting the problem where null is flagged as being an obj
• to be a warning that's turned off by default: done
• to be tested on quotations (last time I tried implementing this, several years ago, I discovered that in quotations all generics get instantiated as obj; my cursory testing just now hasn't thrown that up, but I must have been thinking of something!)
• to be able to assign ranges to every case; currently we get a whole lot of D:\a\_work\1\s\src\Compiler\unknown(1,1): error FS3524: A type… This is essentially done as of Add fallback range to more typars #15301 ; there's still a case (probably in illib.fs) where a range is its default value, but I was unable to track it down, and it seems very rare.

Status of PR: I think this is good to go. When compiling the compiler, the errors have essentially all been explicit uses of null, the underscore in a typedefof<_>, or (rarely) somewhere like static member inline Make (f: obj -> string) = ValueConverter(box (f: obj -> string)) which for some reason required f to be annotated on the right-hand side. There is still at least one place, probably in illib.fs, where we have a type parameter with no range (so we just get the useless diagnostic "unknown(1,1)").

[Smaug123]

@dsyme Do you have any opinions on the treatment of Object.ReferenceEquals(x, null), where null is inferred as obj? I don't see how to suppress the "obj inferred" warning for an explicit use of null without plumbing a lot more information through, whereas it might be pretty noisy if we choose not to suppress it in that case.

[dsyme]

@dsyme Do you have any opinions on the treatment of Object.ReferenceEquals(x, null), where null is inferred as obj? I don't see how to suppress the "obj inferred" warning for an explicit use of null without plumbing a lot more information through, whereas it might be pretty noisy if we choose not to suppress it in that case.

It's this sort of thing that is background to why we don't warn by default for this condition. I'm not sure what would be the best solution - it's perfectly reasonable to infer obj in many cases of F# programming and I don't think you can plug the holes one by one.

The opt-in warning is still likely to be useful in some scenarios so I'm OK with it, but I have no clear idea how often it would trigger in the scenarios where people really care about this.

[Smaug123]

Would you prefer me to go through the F# compiler and get it free of this warning? There will be some noise with e.g. typedefof and null.

[dsyme]

@Smaug123 No, we don't need to worry about this warning in the compiler, thanks

[Smaug123]

@dsyme Would you mind please taking a look at the diff of 97ffb68 and letting me know whether this is a path I should pursue? It does work as a way of giving better type information rather than just unknown(1-1) which otherwise arises in e.g. this test, with one bug arising (I think) because the static member constraint of "has +" gets performed after we warn that this was never refined away from obj. I really don't know how to approach this other than by threading the information all the way up and back down into the constraint solver :/

This also uncovered what could be latent bugs in the tests, in that any error with range0 as its associated range is just silently ignored by |> withDiagnostic and friends; see

fsharp/src/Compiler/Symbols/FSharpDiagnostic.fs

Line 199 in e063dd2

if allErrors || fileName = mainInputFileName || fileName = TcGlobals.DummyFileNameForRangesWithoutASpecificLocation then

[En3Tho]

Will this warn on any inference of obj? I mean on parameters, locals etc? I'm really interested in warning that indicates that "obj" was inferred as type argument of some function call but not really with locals/parameters. Like:

// obj is a perfectly okay fit here from a type sense but it's a high chance bug
JsonSerializer.Deserialize(someString) |> someGenericFunction

[Smaug123]

I'm reasonably happy with this - running against the compiler shows a few oddities listed above, but not all that many.

[Smaug123]

I think this is ready to go. (@nojaf or @safesparrow, how easy would it be for us to make a custom build of this compiler within G-Research and see what it does to my team's monorepo?)

[safesparrow]

Once the change is in a preview SDK, it can be trivially tested internally.
I'm not entirely sure how often the preview SDK is published though.

We also built from source in the past, although not without custom modifications to allow for no internet access.
The dotnet/fsharp repository is mirrored internally, but if the change is not yet merged, you might have to download the source code of the fork manually.
The recent change in the compiler that allows building without the Arcade and bootstrap compiler should make it simpler than before to build internally and require almost no modifications of the source/scripts.
Then it's a matter of pointing your solution build to a custom compiler through the means of setting a few MSBuild props.

[Smaug123]

Thanks - sounds like the easiest way to stress test on our code is just to merge and fix forward if necessary, then.

[nojaf]

@Smaug123 if this get's merged we could try a nightly dotnet 8 SDK to test this inside GR.