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

Error on unreachable cases in match expressions and illegal as expressions. #2289

Merged
merged 2 commits into from Oct 27, 2017

Conversation

Projects
None yet
3 participants
@mfelsche
Contributor

mfelsche commented Oct 20, 2017

this checks for exhaustive matches that have an else clause, which is unreachable,
this also checks for cases that are unreachable because previous cases are already exhaustive,

this also affects the 'as' clause as it is rewritten to a match internally:

this checks for 'as' clauses that try to cast to the same type or a subtype
which would result in an exhaustive match internally and can be considered illegal use of 'as'.

This is a breaking change, but for good. If it affects any pony codebase, then it had a possible source of bugs.

The following examples will be invalid with this PR:

class Foo
  fun ex_match(a: (String| Bool)): String => 
    match a
    | let a: Stringable => a.string()
    | let b: Bool => if b then "true" else "false" end // unreachable already
    else
      "unreachable"
    end
class Foo
   fun subtype_cast(a: String): Stringable ? =>
     a as Stringable // useless as, is actually not partial

   fun eq_cast(a: Array[String]) =>
     let tmp = a as Array[String] // no as needed
    ...
class Foo[A]
  fun alias_cast(a: A!): A ? =>
    a as A
@SeanTAllen

This comment has been minimized.

Show comment
Hide comment
@SeanTAllen

SeanTAllen Oct 20, 2017

Member

Before this gets merged, can you write up release notes for this and add to this PR as a comment.

What I'm looking for is an explanation of invalid code that folks might have in the wild and how they fix it. Also an explanation of why this breaking change is required, and what "eek!" it fixes.

Thanks.

Member

SeanTAllen commented Oct 20, 2017

Before this gets merged, can you write up release notes for this and add to this PR as a comment.

What I'm looking for is an explanation of invalid code that folks might have in the wild and how they fix it. Also an explanation of why this breaking change is required, and what "eek!" it fixes.

Thanks.

error on unreachable code in match expressions
this checks for exhaustive matches that have an else clause, which is unreachable,
this also checks for cases that are unreachable because previous cases are already exhaustive,

this also affects the 'as' clause as it is rewritten to a match internally:

this checks for 'as' clauses that try to cast to the same type or a subtype
which would result in an exhaustive match internally and can be considered illegal use of 'as'.
@mfelsche

This comment has been minimized.

Show comment
Hide comment
@mfelsche

mfelsche Oct 20, 2017

Contributor

Possible Release Notes Entry:


This change is adding some sanity checks to usages of match and as expressions.
It does two things:

Match Expressions

It validates that there is no unreachable code left in your match expressions. This was a subtle source of bugs and left dead code linger in your codebase. Unreachable cases or else clauses are triggering a compiler error now.

Example:

class Foo
  fun ex_match(a: (String| Bool)): String => 
    match a
    | let a: Stringable => a.string()
    | let b: Bool => if b then "true" else "false" end // unreachable already
    else
      "unreachable"
    end

The second branch and the else clause are both unreachable and thus can (and should) be safely removed. In some cases it might instead make sense to rewrite the match, reordering the cases from more specific checks to less specific ones.

As Expressions

It also validates correct use of as, which should only be used to safely increase the specificity of an object's type, that is casting. Previously it was possible to cast to the type of the expression to be casted or to one of its subtypes, which can be achieved by simple assignment. Using as here introduces a false positive partiality. Those incorrect or unnecessary usages of as trigger a compiler error with this change.

Example:

class Foo
   fun subtype_cast(a: String): Stringable ? =>
     a as Stringable // useless as, is actually not partial

   fun eq_cast(a: Array[String]) =>
     let tmp = a as Array[String] // no as needed
     ...

This error can easily be fixed by removing the as as it is not necessary in both cases.

Contributor

mfelsche commented Oct 20, 2017

Possible Release Notes Entry:


This change is adding some sanity checks to usages of match and as expressions.
It does two things:

Match Expressions

It validates that there is no unreachable code left in your match expressions. This was a subtle source of bugs and left dead code linger in your codebase. Unreachable cases or else clauses are triggering a compiler error now.

Example:

class Foo
  fun ex_match(a: (String| Bool)): String => 
    match a
    | let a: Stringable => a.string()
    | let b: Bool => if b then "true" else "false" end // unreachable already
    else
      "unreachable"
    end

The second branch and the else clause are both unreachable and thus can (and should) be safely removed. In some cases it might instead make sense to rewrite the match, reordering the cases from more specific checks to less specific ones.

As Expressions

It also validates correct use of as, which should only be used to safely increase the specificity of an object's type, that is casting. Previously it was possible to cast to the type of the expression to be casted or to one of its subtypes, which can be achieved by simple assignment. Using as here introduces a false positive partiality. Those incorrect or unnecessary usages of as trigger a compiler error with this change.

Example:

class Foo
   fun subtype_cast(a: String): Stringable ? =>
     a as Stringable // useless as, is actually not partial

   fun eq_cast(a: Array[String]) =>
     let tmp = a as Array[String] // no as needed
     ...

This error can easily be fixed by removing the as as it is not necessary in both cases.

@mfelsche

This comment has been minimized.

Show comment
Hide comment
@mfelsche

mfelsche Oct 20, 2017

Contributor

This finally fixes #2253

Contributor

mfelsche commented Oct 20, 2017

This finally fixes #2253

Show outdated Hide outdated src/libponyc/expr/operator.c
@jemc

jemc approved these changes Oct 27, 2017

@jemc jemc changed the title from error on unreachable code in match expressions and illegal as expressions to Error on unreachable cases in match expressions and illegal as expressions. Oct 27, 2017

@mfelsche mfelsche merged commit e256eed into master Oct 27, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

ponylang-main added a commit that referenced this pull request Oct 27, 2017

@mfelsche mfelsche deleted the exhaustive-match-else branch Oct 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment