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

Extend type filtering of conditional clauses to arbitrary logical connectives #10147

Merged
merged 3 commits into from Jan 12, 2021

Conversation

HertzDevil
Copy link
Contributor

@HertzDevil HertzDevil commented Dec 28, 2020

Fixes #8864. This PR makes the following cases work:

x = 1 || 1.0 || 'a' || ""

# when-clauses of >= 3 types now work, including else-branches of the case statement
case x
when Int32, Float64, Char
  typeof(x) # => Int32 | Float64 | Char
else
  typeof(x) # => String
end

# negations of disjunctions now work (same as `unless x.is_a?(Int32) || x.is_a?(Float64)` which already worked)
if !(x.is_a?(Int32) || x.is_a?(Float64))
  typeof(x) # => Char | String
else
  typeof(x) # => Int32 | Float64
end

# the de Morgan equivalent of the above also works now
if !x.is_a?(Int32) && !x.is_a?(Float64)
  typeof(x) # => Char | String
else
  typeof(x) # => Int32 | Float64
end

The fix comes in 3 parts:

  • TypeFilters now always keeps track of the positive and the negative constraints that can be applied to variables, because the negative constraint may provide information to further filter compositions that are unavailable in the positive constraint, or vice versa. (Previously MainVisitor was hardcoded to handle one layer of || expressions.) Consider the condition x.is_a?(T) || f, where x is a variable and f is a call; nothing can be said about the type of x in the then-branch, but we know that !x.is_a?(T) in the else-branch, so we can rely on this fact if we add a ||, &&, or ! to that condition. Negating a TypeFilters is as simple as swapping its positive and negative constraints, whereas for conjunction and disjunction the two constraints are combined separately using de Morgan's laws.
  • Normally the positive and negative constraints of a TypeFilters are complementary to each other, but there is one exception: if the condition is of the form x = f, the truthiness of x and f must be the same, i.e. !!x == !!f. Currently, Crystal assumes the then-branch's constraint is !!x && !!f, so the else-branch's becomes !x || !f; but for assignments inside the condition, we can strengthen the latter constraint to !x && !f. It doesn't matter if x is a temporary variable; this PR makes no distinction of temporary variables generated by the literal expander.
  • Given the nested condition x || y || z where x is a variable, Crystal expands it to (__temp_1 = (x ? x : y)) ? __temp_1 : z. Before this PR, the resulting positive constraint would be (!!x || !!y) || !!__temp_1 || !!z, where the presence of __temp_1, coming from the expanded then-branch, means that x, y, and z can all be falsey based on this constraint alone. This PR eliminates the then-branch in the filter composition, because for || it is always equivalent to the condition, but less elaborated. (A similar argument can be made about the else-branch generated from && expressions.)

This patch also refactors @type_filters so that it is non-nilable (an empty constraint is handled the same as nil and we are already resetting this ivar frequently enough).

@HertzDevil
Copy link
Contributor Author

About this test (and the one immediately below):

it "doesn't restrict || else in sub && (right)" do
  assert_type(%(
    def foo
      a = 1 || nil
      if false || (!a && false)
        return 1
      end
      a
    end
    foo
    )) { nilable int32 }
end

The negation of the condition is !false && (a || !false), whose truthiness cannot be inferred solely from a (it is assumed false can be replaced by any other call), so indeed the else-branch doesn't constrain a's type even after this PR. Any suggestions for better test cases are welcome.

@asterite
Copy link
Member

This is great!! And the code ends up being even simpler than before.

I don't have time right now to review this, but I will maybe in two weeks or a bit more.

Co-authored-by: Sijawusz Pur Rahnama <sija@sija.pl>
keys.concat(filters2.pos.keys)
keys.concat(filters2.neg.keys)
end
keys.uniq!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💭 I wonder if using a Set here for keys is more performant. In any case we can optimize this later on.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect the size to be typically rather small so uniq!'s small size optimization applies.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I totally forgot about that optimization!

Copy link
Member

@asterite asterite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a game changer. Thank you so much! ❤️ ❤️ ❤️

@asterite asterite added this to the 1.0.0 milestone Jan 12, 2021
@asterite asterite merged commit 17fc290 into crystal-lang:master Jan 12, 2021
@HertzDevil HertzDevil deleted the bug/if-inner-clauses branch January 12, 2021 16:52
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.

Type-matching with case doesn't downcast with >= 3 types
4 participants