Skip to content

Conversation

@vzarytovskii
Copy link
Member

Description

A very common feature of F# - pattern matching - has a extremely bad regression in 6.0.300. This regression results in a pattern silently runs the wrong code branch at runtime.

PR
Issue

Customer Impact

The effect on users is that applications will behave incorrectly at runtime - incorrect logic is executed. This will be particularly difficult for users because the resulting behavior is unusual - a derived class may match a base class. We expect this to impact a significant number of users and it has already been reported independently by two different users.

We build the compiler with an earlier version of the compiler so the current shipping compiler is not affected by this regression.

One way this problem may occur is:

type A() = class end
type B() = inherit A()
type C() = inherit A()

let toName (x: obj) =
match x with
| :? A when false -> "not possible"
| :? B -> "B"
| :? C -> "C" // there will be a warning here in the IDE/build output
| _ -> "unknown"

toName (C()) // This will print "B", which is clearly wrong

Regression

Yes. Introduced in 6.0.300.

Testing

The error was in the optimizer.

234 new tests were added to cover this scenario and many adjacent scenarios in pattern matching. Tests now cover:

  • Subtype analysis and relationships
  • Guard clauses and how they interact with sub-typing checks
  • Sealed types and how they interact with inheritance hierarchy checks
  • Multi-column (multi-value) matches and how those interact with exhaustivity checks

Risk

The risk of taking this fix is isolated to F#. Within that context of F#, the fix is straightforward and limited to the pattern matching optimizer and a single kind of pattern. As such, the risk to F# is small.

@kasperk81
Copy link
Contributor

@vzarytovskii, change section is empty. what is the <null> commit 4948e2a?

@baronfel
Copy link
Member

This PR was created with an empty commit so that the problem could be discussed in tactics. The maestro build with the fix has just finished and the team will update this PR shortly for the actual PR build pipeline.

@baronfel
Copy link
Member

Closing in favor of the maestro update PR #25617, which does what this PR intended to do.

@baronfel baronfel closed this May 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants