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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from Jan 12, 2021
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5850cb7
Extend type filtering to arbitrary logical connectives
HertzDevil Dec 28, 2020
b83c55a
Make @type_filters nilable again
HertzDevil Jan 6, 2021
279615e
Update src/compiler/crystal/semantic/filters.cr
HertzDevil Jan 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
87 changes: 79 additions & 8 deletions spec/compiler/semantic/if_spec.cr
View file
Open in desktop
Expand Up @@ -102,6 +102,13 @@ describe "Semantic: if" do
)) { union_of bool, int32 }
end

it "restricts type with !var.is_a?(...) and &&" do
assert_type(%(
a = 1 == 1 ? 1 : ""
!a.is_a?(String) && a + 2
)) { union_of bool, int32 }
end

it "restricts with || (#2464)" do
assert_type(%(
struct Int32
Expand Down Expand Up @@ -256,22 +263,22 @@ describe "Semantic: if" do
assert_type(%(
a = 1 || nil
if !a || 1
1
'c'
else
a
end
), inject_primitives: false) { int32 }
), inject_primitives: false) { union_of char, int32 }
end

it "restricts || else (3) (#3266)" do
assert_type(%(
a = 1 || nil
if 1 || !a
1
'c'
else
a
end
), inject_primitives: false) { int32 }
), inject_primitives: false) { union_of char, int32 }
end

it "doesn't restrict || else in sub && (right)" do
Expand Down Expand Up @@ -306,7 +313,7 @@ describe "Semantic: if" do
)) { nilable int32 }
end

it "doesn't restrict || else in sub || (right)" do
it "restricts || else in sub || (right)" do
assert_type(%(
def foo
a = 1 || nil
Expand All @@ -319,10 +326,10 @@ describe "Semantic: if" do
end

foo
)) { nilable int32 }
)) { int32 }
end

it "doesn't restrict || else in sub || (left)" do
it "restricts || else in sub || (left)" do
assert_type(%(
def foo
a = 1 || nil
Expand All @@ -335,7 +342,71 @@ describe "Semantic: if" do
end

foo
)) { nilable int32 }
)) { int32 }
end

it "restricts && else in sub && (right)" do
assert_type(%(
def foo
a = 1 || nil

if !a && (!a && !a)
return 1
end

a
end

foo
)) { int32 }
end

it "restricts && else in sub && (left)" do
assert_type(%(
def foo
a = 1 || nil

if (!a && !a) && !a
return 1
end

a
end

foo
)) { int32 }
end

it "restricts || of more than 2 clauses (#8864)" do
assert_type(%(
def foo
a = 1 || 2.0 || 'c' || ""

if a.is_a?(Float64) || a.is_a?(Char) || a.is_a?(String)
return 1
end

a
end

foo
)) { int32 }
end

it "restricts && of !var.is_a(...)" do
assert_type(%(
def foo
a = 1 || 2.0 || 'c'

if !a.is_a?(Int32) && !a.is_a?(Float64)
return true
end

a
end

foo
)) { union_of int32, float64, bool }
end

it "doesn't consider nil type in else branch with if with && (#7434)" do
Expand Down
150 changes: 84 additions & 66 deletions src/compiler/crystal/semantic/filters.cr
View file
Open in desktop
Expand Up @@ -32,16 +32,20 @@ module Crystal
abstract class TypeFilter
def self.and(type_filter1, type_filter2)
if type_filter1 == type_filter2
return type_filter1
else
type_filter1
elsif type_filter1 && type_filter2
AndTypeFilter.new(type_filter1, type_filter2)
elsif type_filter1
type_filter1
elsif type_filter2
type_filter2
end
end

def self.or(type_filter1, type_filter2)
if type_filter1 == type_filter2
return type_filter1
else
type_filter1
elsif type_filter1 && type_filter2
OrTypeFilter.new(type_filter1, type_filter2)
end
end
Expand Down Expand Up @@ -83,6 +87,11 @@ module Crystal
type
end

def not
# !(a && b) -> !a || !b
TypeFilter.or(@filter1.not, @filter2.not)
end

def ==(other : self)
@filter1 == other.@filter1 && @filter2 == other.@filter2
end
Expand All @@ -107,6 +116,11 @@ module Crystal
res
end

def not
# !(a || b) -> !a && !b
TypeFilter.and(@filter1.not, @filter2.not)
end

def ==(other : self)
@filter1 == other.@filter1 && @filter2 == other.@filter2
end
Expand Down Expand Up @@ -249,102 +263,106 @@ module Crystal
end

struct TypeFilters
def initialize
@filters = {} of String => TypeFilter
protected getter pos = {} of String => TypeFilter
protected getter neg = {} of String => TypeFilter

protected def initialize
end

protected def initialize(*, @pos, @neg)
end

def self.new(node, filter)
new_filter = new
new_filter[node.name] = filter
new_filter
new_filters = new
new_filters.pos[node.name] = filter
new_filters.neg[node.name] = filter.not
new_filters
end

def self.truthy(node)
new node, TruthyFilter.instance
end

def self.and(filters1, filters2)
if filters1 && filters2
new_filters = TypeFilters.new
all_keys = (filters1.keys + filters2.keys).uniq!
all_keys.each do |name|
filter1 = filters1[name]?
filter2 = filters2[name]?
if filter1 && filter2
new_filters[name] = TypeFilter.and(filter1, filter2)
elsif filter1
new_filters[name] = filter1
elsif filter2
new_filters[name] = filter2
end
return nil if filters1.nil? && filters2.nil?

new_filters = new
common_keys(filters1, filters2).each do |name|
if filter = TypeFilter.and(filters1.try(&.pos[name]?), filters2.try(&.pos[name]?))
new_filters.pos[name] = filter
end
if filter = TypeFilter.or(filters1.try(&.neg[name]?), filters2.try(&.neg[name]?))
new_filters.neg[name] = filter
end
new_filters
elsif filters1
filters1
else
filters2
end
new_filters
end

def self.or(filters1, filters2)
if filters1 && filters2
new_filters = TypeFilters.new
all_keys = (filters1.keys + filters2.keys).uniq!
all_keys.each do |name|
filter1 = filters1[name]?
filter2 = filters2[name]?
if filter1 && filter2
new_filters[name] = TypeFilter.or(filter1, filter2)
end
return nil if filters1.nil? && filters2.nil?

new_filters = new
common_keys(filters1, filters2).each do |name|
if filter = TypeFilter.or(filters1.try(&.pos[name]?), filters2.try(&.pos[name]?))
new_filters.pos[name] = filter
end
if filter = TypeFilter.and(filters1.try(&.neg[name]?), filters2.try(&.neg[name]?))
new_filters.neg[name] = filter
end
new_filters
else
nil
end
new_filters
end

def self.and(filters1, filters2, filters3)
and(filters1, and(filters2, filters3))
end
def self.not(filters)
return nil if filters.nil?

def self.or(filters1, filters2, filters3)
or(filters1, or(filters2, filters3))
TypeFilters.new pos: filters.neg.dup, neg: filters.pos.dup
end

def [](name)
@filters[name]
end
# If we have
#
# if a = b
# ...
# end
#
# then `a` and `b` must have the same truthiness. Thus we can strengthen the
# negation of the condition from `!a || !b` to `!a && !b`, which usually
# provides a stricter filter.
def self.assign_var(filters, target)
if filters.nil?
return new target, TruthyFilter.instance
HertzDevil marked this conversation as resolved.
Show resolved Hide resolved
end

def []?(name)
@filters[name]?
end
name = target.name
filter = TruthyFilter.instance

def []=(name, filter)
@filters[name] = filter
new_filters = filters.dup
new_filters.pos[name] = TypeFilter.and(new_filters.pos[name]?, filter).not_nil!
new_filters.neg[name] = TypeFilter.and(new_filters.neg[name]?, filter.not).not_nil!
new_filters
end

def each
@filters.each do |key, value|
pos.each do |key, value|
yield key, value
end
end

def keys
@filters.keys
def dup
TypeFilters.new pos: pos.dup, neg: neg.dup
end

def not
filters = TypeFilters.new
each do |key, value|
filters[key] = value.not
private def self.common_keys(filters1, filters2)
keys = [] of String
if filters1
keys.concat(filters1.pos.keys)
keys.concat(filters1.neg.keys)
end
filters
end

# Returns true if this filter is only applied to
# a temporary variable created by the compiler
def temp_var?
@filters.size == 1 && @filters.first_key.starts_with?("__temp_")
if filters2
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!

end
end
end