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

Compiler warning when obj is inferred as a type #696

Closed
5 tasks done
Smaug123 opened this issue Sep 21, 2018 · 10 comments · Fixed by dotnet/fsharp#13298
Closed
5 tasks done

Compiler warning when obj is inferred as a type #696

Smaug123 opened this issue Sep 21, 2018 · 10 comments · Fixed by dotnet/fsharp#13298
Labels

Comments

@Smaug123
Copy link

Smaug123 commented Sep 21, 2018

I propose we create a compiler warning, off-by-default, that warns when the typechecker infers a type to be obj without annotation.

Ideally, if there is some reason to expect that something is of type obj, then we would not warn. For example, the following code should not generate a warning, because a is constrained to be obj by the type argument to unbox, and b is constrained to be obj because it is of the same type as a.

let a = 5 |> unbox<obj>
let b = a

However, we would warn in the following case, where a is inferred to be obj because that's the most general thing it could be:

let f<'b> () : 'b =
    let a = failwith ""
    a |> unbox

The existing way of approaching this problem in F# is to ignore it and assume that you intended obj whenever obj is inferred (unless there's some other reason to think that you didn't mean it, such as unnecessary specialisation of a generic type as indicated by FS0064).

Pros and Cons

The advantages of making this adjustment to F# are that it causes us to catch a class of bug we currently can't catch. I have never encountered an instance where the compiler has inferred obj and my code was correct; using obj at all is sufficiently weird that it seems reasonable to expect it to be signposted with a type annotation somewhere (much like mutable is a required signpost for mutability). However, since this is a feature of F#, it seems reasonable to disable the warning by default.

The disadvantages of making this adjustment to F# are a slight increase in complexity during typecheck, since we need to work out whether a type has been specialised.

Extra information

Estimated cost (XS, S, M, L, XL, XXL): S

Related suggestions: none

Affidavit (please submit!)

Please tick this by placing a cross in the box:

  • This is not a question (e.g. like one you might ask on stackoverflow) and I have searched stackoverflow for discussions of this issue
  • I have searched both open and closed suggestions on this site and believe this is not a duplicate
  • This is not something which has obviously "already been decided" in previous versions of F#. If you're questioning a fundamental design decision that has obviously already been taken (e.g. "Make F# untyped") then please don't submit it.

Please tick all that apply:

  • This is not a breaking change to the F# language design
  • I or my company would be willing to help implement and/or test this
@isaacabraham
Copy link

I know where you're coming from with this one. But wouldn't you typically catch this as an error almost immediately afterwards when you try to bind the result to e.g. input of another function that expects a value of a specific type?

@Smaug123
Copy link
Author

I know where you're coming from with this one. But wouldn't you typically catch this as an error almost immediately afterwards when you try to bind the result to e.g. input of another function that expects a value of a specific type?

Typically, yes. However, in the example above, where we pass into unbox, it's turned into a runtime failure. In fact this situation arises pretty frequently when I use the skolemisation trick to encode existential types (see #567 for discussion, in which it seems pretty clear that we're not getting existential types in F# any time soon). This is already a boilerplate-heavy pattern; to make this encoding truly type-safe without using unbox, the boilerplate grows by about another third in size, so in practice I end up just using unbox instead; and this opens the door to the bug I'm trying to prevent with this warning.

@cartermp
Copy link
Member

cartermp commented Oct 3, 2018

I agree with the spirit of this - just today, in fact, I was helping someone with their code constraining to obj. They had done a partial refactoring, and instead of fully changing their code to adjust to a new type, the used a cast which resulted in the constraint. I suspect it would have failed at runtime.

However, this is a breaking change, and there are some valid use cases for obj as the type (even if it's been inferred).

@Smaug123
Copy link
Author

Smaug123 commented Oct 3, 2018

@cartermp This is why I propose making the warning be off by default: so that users must opt into the breaking change.

@dsyme
Copy link
Collaborator

dsyme commented Oct 12, 2018

@Smaug123 @cartermp I'd be happy with the addition of an off-by-default compiler warning.

@Smaug123
Copy link
Author

I looked at this, by the way, and got something "working" (caught a couple of bugs in our existing codebase!) However, there turns out to be some information dropped by the compiler related to inferred type variables: in some situations, the compiler will simply forget where in the code the type was inferred. This means you very often just get a warning of the form "Inferred obj somewhere in this .fsproj", with no further information like file or line number. That makes this all but useless in practice. (I asked Don about this, and he said this was a perf optimisation at some point in the past, but that it was unclear whether the optimisation is actually needed.)

I hacked the extra information in, in some situations, but never had the time to find a real solution to this.

There are also a couple of instances where the warning is completely unhelpful, primarily inside quotations (where every generic is instantiated to obj anyway). The compiler itself won't build with this warning enabled as an error, since there is some automatically generated code somewhere in the process which contains an explicit use of null (which is inferred to be an obj).

@abelbraaksma
Copy link
Member

@Smaug123, awesome that you're working on this! I've been coaching a team of programmers (relatively) new to F# and this is one of those cases that confuses the heck out of them. Adding that warning would certainly help alleviate some of that burden.

@Smaug123
Copy link
Author

The PR is sort of ready, except that it has a few quirks.

  • null is often inferred as obj and there's not really much we can do about it (though the user can annotate it explicitly as null : obj if they just want to get rid of the warning).
  • typedefof<list<_>> is a very common pattern, and _ is always implicitly obj in there (again, the user can write list<obj> if they want to get rid of the warning).
  • There are various places where types just don't have associated ranges for some reason - I think Don mentioned that this was an optimisation introduced ages ago which may no longer be necessary, but I don't know where that optimisation is, and it means sometimes we only get unknown(1,1) as the location of the warning. A warning that says "er, somewhere in the codebase I inferred obj" is not a very good one, even if it happens quite rarely!

@Smaug123
Copy link
Author

Smaug123 commented Jul 29, 2022

I spoke too soon: it works only by coincidence. In fact it's broken in the presence of SRTPs (e.g. let add x y = x + y, where the type variable is inferred to int); the brokenness was actually just hidden by a combination of two possibly-bugs (namely: the compiler bug that the type variable in that example has no range data associated with it, and the test harness bug that warnings/errors which have no range data associated with them are silently dropped).

Maybe the false positives arising from this are acceptable?

@Smaug123
Copy link
Author

An error message is now reserved for this (dotnet/fsharp#14642).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants