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

Variables in match shadowing types should trigger error instead of warning #826

Open
voronoipotato opened this issue Jan 13, 2020 · 7 comments
Open

Comments

@voronoipotato
Copy link

@voronoipotato voronoipotato commented Jan 13, 2020

Variables in match shadowing types should trigger error instead of warning

I propose that when we match, if there is an uppercase variable identifier that shadows an existing type that we throw an error. A new F# developer in the F# beginners slack shared some code they were trying, and the issue wasn't instantly obvious. Perhaps it should have been but because it was a warning the new user didn't think it pertinent to include it when asking for help. The code was returning 10.80 instead of the expected 10.35. It did feel like to an extent the type system was sidestepped and there was only warnings in response, since this was a catch-em-all match.

type PizzaSize = 
    |Personal
    |Family
type MeatTopping =
    |Pepperoni
    |Chicken
    |Sausage
    |Bacon
type VeggieTopping =
    |Mushrooms
    |GreenPeppers
    |Onions
    |Tomatoes
    |Pineapple
type Topping =
  |Meat of MeatTopping
  |Veggie of VeggieTopping
type Pizza = {
     Size: PizzaSize
     Toppings: Topping list
}

let getPriceOfPizza pizza =
    let {Size=size; Toppings=toppings} = pizza
    let basePrice = match size with
        |Personal -> 9.0
        |Family -> 18.0
    toppings|> List.map (fun x -> match x with |MeatTopping -> 0.10 | VeggieTopping -> 0.05) |> List.map (fun x -> x * basePrice) |> List.fold (+) basePrice    

[<Fact>]
let PizzaWithMushroomsAndPepperoni () =
    let personalPizza = {Pizza.Size=Pizza.Personal; Pizza.Toppings=[Pizza.Topping.Veggie(Pizza.Mushrooms); Pizza.Topping.Meat(Pizza.Pepperoni)]}
    let price = Pizza.getPriceOfPizza personalPizza
    price |> should equal 10.35 // this is getting 10.80

The existing way of approaching this problem in F# is looking at the warning.
stdin(37,33): warning FS0049: Uppercase variable identifiers should not generally be used in patterns, and may indicate a misspelt pattern name . This is fine for non-beginners but this being a warning and not an error is a footgun for beginners. The original warning itself may also need some work, being clear that what they typed was not actually indeed a "type". I was even discussing making this warning an error with Tyler Hartwig, but I thought a more attainable goal was to make shadowed types an error, as there's lower risk of breaking code.

Pros and Cons

The advantages of making this adjustment to F# are ...

More accessibility for beginners.

The disadvantages of making this adjustment to F# are ...

Potential for breaking existing F# code, though it seems like anyone who has enough code to encounter this in production code could just demote it to a warning.

Extra information

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

Related suggestions: (put links to related suggestions here)

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
@cartermp

This comment has been minimized.

Copy link
Member

@cartermp cartermp commented Jan 13, 2020

For clarity's sake the line is here:

toppings|> List.map (fun x -> match x with |MeatTopping -> 0.10 | VeggieTopping -> 0.05) |> List.map (fun x -> x * basePrice) |> List.fold (+) basePrice 

Or

toppings
|> List.map (fun x ->
    match x with
    | MeatTopping -> 0.10 // Right here
    | VeggieTopping -> 0.05)
|> List.map (fun x -> x * basePrice)
|> List.fold (+) basePrice 

This would be a breaking change to emit an error, since the current warning only says that no other cases will match due to it being a Variable Pattern. But I'm actually in favor of doing an intentional breaking change like this in a new language version, since this most certainly means there is a bug we're obscuring with another warning.

@charlesroddie

This comment has been minimized.

Copy link

@charlesroddie charlesroddie commented Jan 15, 2020

The current warning is exactly right. Uppercase identifiers are discouraged.

The suggestion to make an inner binding change it's behaviour because it's name matches something outside is not right. You have to be able to reason about self contained units of code, and saying this piece of code works provided nothing outside shares the same name is not acceptable. That's the point of scoping.

@abelbraaksma

This comment has been minimized.

Copy link

@abelbraaksma abelbraaksma commented Jan 15, 2020

who has enough code to encounter this in production code could just demote it to a warning.

I don't think we have a mechanism in place that turns an error into a warning.

An error ought to be something that the compiler cannot reason about and cannot fix. A warning should be something that's likely a coding mistake, but is of no consequence per se for the compiler, and that might be intentional.

Treating 'bad coding practices' as compiler errors doesn't seem to me to be the right way to go. However, the learning curve here is steep, we should certainly attempt to make it easier for beginners.

Perhaps a way forward is to put FS0049 in the list of 'treat warnings as errors' upon creation of a new project? And if we improve the message for such errors to include something like 'if this is intentional, remove this warning from the 'treat warnings as error' list.'.

Same is true for shadowing, which I think is an improvement if we can catch these, but existing code may do this deliberately, so making it a new warning, possibly defaulting as error in the same way, may be the way to go.

@cartermp

This comment has been minimized.

Copy link
Member

@cartermp cartermp commented Jan 15, 2020

I don't think existing code does this kind of shadowing deliberately. Variable patterns are of course used deliberately (we do it all the time in the F# compiler) but when it shadows the name of a case on the target type of a pattern match that is most assuredly a bug. I suppose someone could have done that deliberately, but the use case seems so unlikely that I think we'd do more good by emitting a specific warning or error here than keeping the status quo.

@charlesroddie

This comment has been minimized.

Copy link

@charlesroddie charlesroddie commented Jan 15, 2020

Let me expand. We can't allow heuristics about the behaviour of programmers to override logical properties of the language.

The issue in the OP is solved in ways that are not drastic. 1. Read the intellisense. In this case warnings, not only the FS0049 mentioned but also "This rule will never be matched". 2. Annotate function inputs and avoid long chains of |> bindings, to help identify the location of errors. These are two of four pieces of advice that I find answer almost all beginner questions.

Erroring on match identifiers with capital letters would be OK in the sense that it would not turn F# into a logical mess. It would merely break code and lead to syntactic inconsistencies. (let A = () compiles, but let (A,_) = ((),()) may or may not, and match ((),()) with A -> () doesn't.) You would also have to ban type a() = ....

Breaking scoping is much worse. If identifiers starting with capital letters compile, they have to work.

let f() =
    match () with
    | A -> ()

This is a self-contained piece of code which compiles by itself. Therefore wherever it is placed, in any place where functions may be defined, it should still compile and do the same thing. If you add a type A() = class end in the same scope, this should therefore not affect the compilation or function of f.

I am sure someone will write an AI language in which let two = 3 doesn't compile, or compiles and sets two to 2, on the basis that it was probably a mistake. But F# is based on the rule of law, not act-utilitarianism, and these laws need to be simple and preserve the ability to reason about code.

@cartermp

This comment has been minimized.

Copy link
Member

@cartermp cartermp commented Jan 15, 2020

@charlesroddie note that in your example:

  1. type A() = class end isn't being considered here - only when a name shadows the name of a case of a DU that is being matched on.
  2. Your example of placing code somewhere and still compiling isn't quite accurate, as there are numerous examples where shadowing in F# results in pasted code that compiled no longer compiling due to a type mismatch or ambiguity, or hitting a runtime error.

While the latter case can always be explained by deterministic behavior, it does mean that the location in which you place some code can result in a different outcome. This isn't always going to be predictable.

I think an error is a bit extreme here, but this does seem like a candidate for a different kind of warning. We emit warnings for things that are technically valid but are likely to result in an error or unexpected runtime behavior all the time.

@voronoipotato

This comment has been minimized.

Copy link
Author

@voronoipotato voronoipotato commented Jan 15, 2020

To shed the idea that errors must be logical impossibilities and not user experience there already is some precedent for this. While errors are often things that are impossible or illogical, it's incorrect to say that they are strictly for that.

error FS0053: Discriminated union cases and exception labels must be uppercase identifiers

I guess in a broad sense the question we're exploring could be restated as "Should I be able to shadow a type definition with a variable name?". In my opinion it doesn't really make clear sense why I can shadow a type with a variable. The shadowing of the type containing the DU cases then makes it even harder to reason about. I personally think FS0053 makes good rational sense.

type A = A of int
let A = A 7 //weird
let (x:A) = A //also weird...
let f = function | A -> 7 //fails
//error FS0019: This constructor is applied to 0 argument(s) but expects 1
let g = function | B -> 7 //works

type B = | B
let B = 7
let c = B //val z : int = 7 
//currently no way to assign B to a variable, but matching resolves to type B
let h = function | B -> 14 //val h : _arg1:B -> int
h B
//error FS0001: This expression was expected to have type 'B' but here has 'int'
//now I have a function which I cannot pass any arguments to.

This currently appears to me to violate @charlesroddie 's rules about scope. Notice how it doesn't actually fully shadow here. I suspect there's some risk of dragons here....

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

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.