diff --git a/lib/solargraph/api_map.rb b/lib/solargraph/api_map.rb index 26b42ddb47..f336772b23 100755 --- a/lib/solargraph/api_map.rb +++ b/lib/solargraph/api_map.rb @@ -416,7 +416,7 @@ def var_at_location candidates, name, closure, location !pin.visible_at?(closure, location) && !pin.starts_at?(location) end - vars_at_location.inject(&:combine_with) + vars_at_location.inject { |acc, pin| acc.combine_with(pin, location: location) } end # Get an array of class variable pins for a namespace. @@ -706,11 +706,8 @@ def super_and_sub? sup, sub # @todo If two literals are different values of the same type, it would # make more sense for super_and_sub? to return true, but there are a # few callers that currently expect this to be false. - # @sg-ignore flow-sensitive typing should be able to handle redefinition return false if sup.literal? && sub.literal? && sup.to_s != sub.to_s - # @sg-ignore flow sensitive typing should be able to handle redefinition sup = sup.simplify_literals.to_s - # @sg-ignore flow sensitive typing should be able to handle redefinition sub = sub.simplify_literals.to_s return true if sup == sub sc_fqns = sub diff --git a/lib/solargraph/complex_type.rb b/lib/solargraph/complex_type.rb index 5b0e984b25..411a9cb5d5 100644 --- a/lib/solargraph/complex_type.rb +++ b/lib/solargraph/complex_type.rb @@ -224,6 +224,7 @@ def conforms_to? api_map, expected, situation, rules = [], variance: erased_variance(situation) + # @sg-ignore flow sensitive typing needs to handle a self-referential reassignment (x = x.foo) expected = expected.downcast_to_literal_if_possible inferred = downcast_to_literal_if_possible diff --git a/lib/solargraph/parser/flow_sensitive_typing.rb b/lib/solargraph/parser/flow_sensitive_typing.rb index 1606a32e06..cd99b47e54 100644 --- a/lib/solargraph/parser/flow_sensitive_typing.rb +++ b/lib/solargraph/parser/flow_sensitive_typing.rb @@ -9,11 +9,30 @@ class FlowSensitiveTyping # @param ivars [Array] # @param enclosing_breakable_pin [Solargraph::Pin::Breakable, nil] # @param enclosing_compound_statement_pin [Solargraph::Pin::CompoundStatement, nil] - def initialize locals, ivars, enclosing_breakable_pin, enclosing_compound_statement_pin + # @param restricted_names [Array, nil] If given, only + # assert facts about variables with these names, ignoring any + # other variable the analyzed condition happens to mention. + def initialize locals, ivars, enclosing_breakable_pin, enclosing_compound_statement_pin, + restricted_names: nil @locals = locals @ivars = ivars @enclosing_breakable_pin = enclosing_breakable_pin @enclosing_compound_statement_pin = enclosing_compound_statement_pin + @restricted_names = restricted_names + end + + # Assert the facts implied by a condition being true/false over + # the given ranges. Public so that a differently-configured + # instance (see #initialize's restricted_names) can be handed a + # condition to analyze. + # + # @param conditional_node [Parser::AST::Node] + # @param true_ranges [Array] + # @param false_ranges [Array] + # + # @return [void] + def process_condition conditional_node, true_ranges, false_ranges + process_expression(conditional_node, true_ranges, false_ranges) end # @param and_node [Parser::AST::Node] @@ -132,6 +151,8 @@ def process_if if_node, true_ranges = [], false_ranges = [] true_ranges << rest_of_returnable_body if always_leaves_compound_statement?(else_clause) end + assert_after_skipped_or_asgn(conditional_node, then_clause, else_clause) + unless then_clause.nil? # # If the condition is true we can assume things about the then clause @@ -152,7 +173,11 @@ def process_if if_node, true_ranges = [], false_ranges = [] get_node_end_position(else_clause)) end + return if conditional_node.nil? + process_expression(conditional_node, true_ranges, false_ranges) + + process_guarded_reassignment(if_node, conditional_node, then_clause, else_clause) end # @param while_node [Parser::AST::Node] @@ -198,6 +223,120 @@ class << self private + # The standard default-argument idiom reassigns a variable in + # the branch where the guard on that same variable fired: + # + # tasks = ['a'] if tasks.nil? + # tasks.each { ... } + # + # At a use site *after* the conditional, the two incoming paths + # are (a) the guard fired and the clause assigned a new value, + # and (b) the guard did not fire, leaving the original value - + # which the condition tells us something about. Path (a) is + # already handled: the assignment's pin is unioned in. Path (b) + # is what's asserted here - the opposite branch's facts from the + # condition hold over the rest of the enclosing compound + # statement. + # + # The facts are restricted to the variables the clause + # definitely reassigns. Without that restriction a condition + # like `x.nil? || y.nil?` would wrongly narrow `y` after the + # conditional, since the clause only replaced `x`'s value. + # + # @param if_node [Parser::AST::Node] + # @param conditional_node [Parser::AST::Node] + # @param then_clause [Parser::AST::Node, nil] + # @param else_clause [Parser::AST::Node, nil] + # + # @return [void] + def process_guarded_reassignment if_node, conditional_node, then_clause, else_clause + compound_statement_node = enclosing_compound_statement_pin&.node + return if compound_statement_node.nil? + + rest_of_compound_statement = Range.new(get_node_end_position(if_node), + get_node_end_position(compound_statement_node)) + + # the then clause ran only when the condition was true, so the + # path that preserved the original value is the false one - + # and vice versa for the else clause + assert_after_guard(conditional_node, definitely_assigned_names(then_clause), + [], [rest_of_compound_statement]) + assert_after_guard(conditional_node, definitely_assigned_names(else_clause), + [rest_of_compound_statement], []) + end + + # A leaving guard inside a `x ||= ...` body still dominates the + # code after the ||=, but only for x: the body is skipped + # exactly when x was truthy, so both paths reach the same + # conclusion about x. No other variable gets that guarantee, + # hence the restriction to x by name. + # + # @param conditional_node [Parser::AST::Node, nil] + # @param then_clause [Parser::AST::Node, nil] + # @param else_clause [Parser::AST::Node, nil] + # + # @return [void] + def assert_after_skipped_or_asgn conditional_node, then_clause, else_clause + return if conditional_node.nil? + + or_asgn_pin = enclosing_compound_statement_pin + return if or_asgn_pin.nil? + + or_asgn_node = or_asgn_pin.node + return if or_asgn_node.nil? + return unless or_asgn_node.type == :or_asgn + + parent = or_asgn_pin.compound_statement + return if parent.nil? + + parent_node = parent.node + return if parent_node.nil? + + lhs_node = or_asgn_node.children[0] + return if lhs_node.nil? + + name = lhs_node.children[0].to_s + rest = Range.new(get_node_end_position(or_asgn_node), get_node_end_position(parent_node)) + + assert_after_guard(conditional_node, [name], [], [rest]) if always_leaves_compound_statement?(then_clause) + assert_after_guard(conditional_node, [name], [rest], []) if always_leaves_compound_statement?(else_clause) + end + + # @param conditional_node [Parser::AST::Node] + # @param names [Array] + # @param true_ranges [Array] + # @param false_ranges [Array] + # + # @return [void] + def assert_after_guard conditional_node, names, true_ranges, false_ranges + return if names.empty? + + FlowSensitiveTyping.new(locals, ivars, enclosing_breakable_pin, enclosing_compound_statement_pin, + restricted_names: names) + .process_condition(conditional_node, true_ranges, false_ranges) + end + + # Names of the variables this clause assigns on every path + # through it. Only unconditional, plain assignments count - + # anything inside a nested conditional or loop may not run, and + # `||=`/`+=`-style assignments keep the previous value in play. + # + # @param clause_node [Parser::AST::Node, nil] + # + # @return [Array] + def definitely_assigned_names clause_node + return [] if clause_node.nil? + + case clause_node.type + when :lvasgn, :ivasgn + [clause_node.children[0].to_s] + when :begin, :kwbegin + clause_node.children.flat_map { |child| definitely_assigned_names(child) } + else + [] + end + end + # @param pin [Pin::BaseVariable] # @param presence [Range] # @param downcast_type [ComplexType, nil] @@ -205,6 +344,8 @@ class << self # # @return [void] def add_downcast_var pin, presence:, downcast_type:, downcast_not_type: + return if restricted_names && !restricted_names.include?(pin.name) + new_pin = pin.downcast(exclude_return_type: downcast_not_type, intersection_return_type: downcast_type, source: :flow_sensitive_typing, @@ -249,9 +390,62 @@ def process_expression expression_node, true_ranges, false_ranges process_calls(expression_node, true_ranges, false_ranges) process_and(expression_node, true_ranges, false_ranges) process_or(expression_node, true_ranges, false_ranges) + process_parentheses(expression_node, true_ranges, false_ranges) + process_assignment(expression_node, true_ranges, false_ranges) process_variable(expression_node, true_ranges, false_ranges) end + # `(foo)` parses as a one-child :begin wrapping the expression, + # which is how an assignment used as a condition normally shows + # up: `if (md = foo.match(...))`. A multi-statement :begin + # takes its truthiness from the last statement, which isn't + # worth handling here. + # + # @param node [Parser::AST::Node] + # @param true_ranges [Array] + # @param false_ranges [Array] + # + # @return [void] + def process_parentheses node, true_ranges, false_ranges + return unless node.type == :begin && node.children.length == 1 + + child = node.children[0] + return unless child.is_a?(::Parser::AST::Node) + + # @sg-ignore flow sensitive typing doesn't narrow `child` past the guard above + process_expression(child, true_ranges, false_ranges) + end + + # An assignment used as a condition - `if (md = foo.match(...))` + # - evaluates to the value assigned, so the branches tell us the + # same thing about the variable that a bare reference to it + # would. + # + # @param node [Parser::AST::Node] + # @param true_presences [Array] + # @param false_presences [Array] + # + # @return [void] + def process_assignment node, true_presences, false_presences + return unless %i[lvasgn ivasgn].include?(node.type) + + variable_name = node.children[0]&.to_s + return if variable_name.nil? || variable_name.empty? + + # look the variable up at the end of its own assignment, where + # the new value has become visible + pin = find_var(variable_name, get_node_end_position(node)) + return unless pin + + # @type Hash{Pin::BaseVariable => Array ComplexType}>} + if_true = { pin => [{ not_type: ComplexType::NIL }] } + process_facts(if_true, true_presences) + + # @type Hash{Pin::BaseVariable => Array ComplexType}>} + if_false = { pin => [{ type: ComplexType.parse('nil, false') }] } + process_facts(if_false, false_presences) + end + # @param call_node [Parser::AST::Node] # @param method_name [Symbol] # @return [Array(String, String), nil] Tuple of rgument to @@ -298,13 +492,30 @@ def parse_isa isa_node # return type could not be inferred # @return [Solargraph::Pin::LocalVariable, Solargraph::Pin::InstanceVariable, nil] def find_var variable_name, position - if variable_name.start_with?('@') - # @sg-ignore flow sensitive typing needs to handle attrs - ivars.find { |ivar| ivar.name == variable_name && (!ivar.presence || ivar.presence.include?(position)) } - else + pins = variable_name.start_with?('@') ? ivars : locals + # Prefer the pin whose presence starts latest - i.e., the + # most recent assignment reaching this position - rather + # than the first-declared pin for this name. Multiple pins + # can match (e.g. a variable's original declaration and a + # later reassignment both have presences that include this + # position), and picking the wrong one here would narrow the + # stale, superseded pin instead of the current one. + # + # Exclude pins whose own assignment is still being evaluated + # at this position (e.g. the receiver inside its own RHS, + # such as `baz ||= begin ... end`) - that pin's value isn't + # available yet, so its presence including this position + # would otherwise make it a false match ahead of the pin it's + # about to supersede. + matches = pins.select do |pin| + next false unless pin.name == variable_name # @sg-ignore flow sensitive typing needs to handle attrs - locals.find { |pin| pin.name == variable_name && (!pin.presence || pin.presence.include?(position)) } + next false unless !pin.presence || pin.presence.include?(position) + + other_loc = Location.new(pin.location&.filename, Range.new(position, position)) + !pin.within_own_assignment?(other_loc) end + matches.max_by { |pin| pin.presence&.start || Position.new(0, 0) } end # @param isa_node [Parser::AST::Node] @@ -465,7 +676,8 @@ def always_leaves_compound_statement? clause_node %i[return raise next redo retry].include?(clause_node&.type) end - attr_reader :locals, :ivars, :enclosing_breakable_pin, :enclosing_compound_statement_pin + attr_reader :locals, :ivars, :enclosing_breakable_pin, :enclosing_compound_statement_pin, + :restricted_names end end end diff --git a/lib/solargraph/parser/parser_gem/node_processors/and_node.rb b/lib/solargraph/parser/parser_gem/node_processors/and_node.rb index 83f14a4157..35edda8384 100644 --- a/lib/solargraph/parser/parser_gem/node_processors/and_node.rb +++ b/lib/solargraph/parser/parser_gem/node_processors/and_node.rb @@ -8,7 +8,20 @@ class AndNode < Parser::NodeProcessor::Base include ParserGem::NodeMethods def process - process_children + # the rhs of `a && b` only executes if `a` is truthy, so + # any assignment there isn't guaranteed to have executed + lhs, rhs = node.children + NodeProcessor.process(lhs, region, pins, locals, ivars) + rhs_cs = Solargraph::Pin::CompoundStatement.new( + location: get_node_location(rhs), + closure: region.closure, + compound_statement: region.compound_statement, + conditional: true, + node: rhs, + source: :parser + ) + pins.push rhs_cs + NodeProcessor.process(rhs, region.update(compound_statement: rhs_cs), pins, locals, ivars) FlowSensitiveTyping.new(locals, ivars, diff --git a/lib/solargraph/parser/parser_gem/node_processors/args_node.rb b/lib/solargraph/parser/parser_gem/node_processors/args_node.rb index 9a22b8edd0..a45250b097 100644 --- a/lib/solargraph/parser/parser_gem/node_processors/args_node.rb +++ b/lib/solargraph/parser/parser_gem/node_processors/args_node.rb @@ -23,6 +23,11 @@ def process # @sg-ignore Need to add nil check here presence: callable.location.range, decl: get_decl(u), + # a default value expression is only assigned + # conditionally (when the caller omits the arg), + # so it shouldn't be treated as a guaranteed + # override of the declared @param type + definite: false, source: :parser ) callable.parameters.push locals.last diff --git a/lib/solargraph/parser/parser_gem/node_processors/block_node.rb b/lib/solargraph/parser/parser_gem/node_processors/block_node.rb index 750bb99294..08add69d9c 100644 --- a/lib/solargraph/parser/parser_gem/node_processors/block_node.rb +++ b/lib/solargraph/parser/parser_gem/node_processors/block_node.rb @@ -20,6 +20,11 @@ def process block_pin = Solargraph::Pin::Block.new( location: location, closure: region.closure, + compound_statement: region.compound_statement, + # a block's body may execute zero or multiple times (e.g. + # Enumerable#each), so an assignment inside it is never + # guaranteed to have executed + conditional: true, node: node, context: context, receiver: node.children[0], @@ -28,7 +33,7 @@ def process source: :parser ) pins.push block_pin - process_children region.update(closure: block_pin) + process_children region.update(closure: block_pin, compound_statement: block_pin) end private diff --git a/lib/solargraph/parser/parser_gem/node_processors/def_node.rb b/lib/solargraph/parser/parser_gem/node_processors/def_node.rb index f45f5544df..b6e6137d68 100644 --- a/lib/solargraph/parser/parser_gem/node_processors/def_node.rb +++ b/lib/solargraph/parser/parser_gem/node_processors/def_node.rb @@ -15,6 +15,7 @@ def process methpin = Solargraph::Pin::Method.new( location: get_node_location(node), closure: region.closure, + compound_statement: region.compound_statement, name: name, context: method_context, comments: comments_for(node), @@ -51,7 +52,7 @@ def process else pins.push methpin end - process_children region.update(closure: methpin, scope: methpin.scope) + process_children region.update(closure: methpin, scope: methpin.scope, compound_statement: methpin) end end end diff --git a/lib/solargraph/parser/parser_gem/node_processors/defs_node.rb b/lib/solargraph/parser/parser_gem/node_processors/defs_node.rb index 09679c7f76..9690fcf879 100644 --- a/lib/solargraph/parser/parser_gem/node_processors/defs_node.rb +++ b/lib/solargraph/parser/parser_gem/node_processors/defs_node.rb @@ -22,6 +22,7 @@ def process pins.push Solargraph::Pin::Method.new( location: loc, closure: closure, + compound_statement: region.compound_statement, name: node.children[1].to_s, comments: comments_for(node), scope: :class, @@ -29,7 +30,7 @@ def process node: node, source: :parser ) - process_children region.update(closure: pins.last, scope: :class) + process_children region.update(closure: pins.last, scope: :class, compound_statement: pins.last) end end end diff --git a/lib/solargraph/parser/parser_gem/node_processors/if_node.rb b/lib/solargraph/parser/parser_gem/node_processors/if_node.rb index 0b9a75e774..b9d357a816 100644 --- a/lib/solargraph/parser/parser_gem/node_processors/if_node.rb +++ b/lib/solargraph/parser/parser_gem/node_processors/if_node.rb @@ -8,40 +8,50 @@ class IfNode < Parser::NodeProcessor::Base include ParserGem::NodeMethods def process - FlowSensitiveTyping.new(locals, - ivars, - enclosing_breakable_pin, - enclosing_compound_statement_pin).process_if(node) condition_node = node.children[0] if condition_node pins.push Solargraph::Pin::CompoundStatement.new( location: get_node_location(condition_node), closure: region.closure, + compound_statement: region.compound_statement, node: condition_node, source: :parser ) NodeProcessor.process(condition_node, region, pins, locals, ivars) end + # after the condition, so that a variable the condition + # itself assigns (`if (md = foo.match(...))`) already has a + # pin to assert facts about + FlowSensitiveTyping.new(locals, + ivars, + enclosing_breakable_pin, + enclosing_compound_statement_pin).process_if(node) then_node = node.children[1] if then_node - pins.push Solargraph::Pin::CompoundStatement.new( + then_cs = Solargraph::Pin::CompoundStatement.new( location: get_node_location(then_node), closure: region.closure, + compound_statement: region.compound_statement, + conditional: true, node: then_node, source: :parser ) - NodeProcessor.process(then_node, region, pins, locals, ivars) + pins.push then_cs + NodeProcessor.process(then_node, region.update(compound_statement: then_cs), pins, locals, ivars) end else_node = node.children[2] if else_node - pins.push Solargraph::Pin::CompoundStatement.new( + else_cs = Solargraph::Pin::CompoundStatement.new( location: get_node_location(else_node), closure: region.closure, + compound_statement: region.compound_statement, + conditional: true, node: else_node, source: :parser ) - NodeProcessor.process(else_node, region, pins, locals, ivars) + pins.push else_cs + NodeProcessor.process(else_node, region.update(compound_statement: else_cs), pins, locals, ivars) end true diff --git a/lib/solargraph/parser/parser_gem/node_processors/lvasgn_node.rb b/lib/solargraph/parser/parser_gem/node_processors/lvasgn_node.rb index 63e2c55dcd..8fd0212074 100644 --- a/lib/solargraph/parser/parser_gem/node_processors/lvasgn_node.rb +++ b/lib/solargraph/parser/parser_gem/node_processors/lvasgn_node.rb @@ -19,6 +19,9 @@ def process assignment: node.children[1], comments: comments_for(node), presence: presence, + # false inside an if/while/rescue body, which may not run + definite: !region.compound_statement.conditional, + compound_statement: region.compound_statement, source: :parser ) process_children diff --git a/lib/solargraph/parser/parser_gem/node_processors/namespace_node.rb b/lib/solargraph/parser/parser_gem/node_processors/namespace_node.rb index 7cecd22c09..5ffedbc88f 100644 --- a/lib/solargraph/parser/parser_gem/node_processors/namespace_node.rb +++ b/lib/solargraph/parser/parser_gem/node_processors/namespace_node.rb @@ -20,6 +20,7 @@ def process type: node.type, location: loc, closure: region.closure, + compound_statement: region.compound_statement, name: name, comments: comments, visibility: :public, @@ -35,7 +36,7 @@ def process source: :parser ) end - process_children region.update(closure: nspin, visibility: :public) + process_children region.update(closure: nspin, visibility: :public, compound_statement: nspin) end private diff --git a/lib/solargraph/parser/parser_gem/node_processors/or_node.rb b/lib/solargraph/parser/parser_gem/node_processors/or_node.rb index 6c54f1c8c1..7deaf54913 100644 --- a/lib/solargraph/parser/parser_gem/node_processors/or_node.rb +++ b/lib/solargraph/parser/parser_gem/node_processors/or_node.rb @@ -8,7 +8,20 @@ class OrNode < Parser::NodeProcessor::Base include ParserGem::NodeMethods def process - process_children + # the rhs of `a || b` only executes if `a` is falsy, so + # any assignment there isn't guaranteed to have executed + lhs, rhs = node.children + NodeProcessor.process(lhs, region, pins, locals, ivars) + rhs_cs = Solargraph::Pin::CompoundStatement.new( + location: get_node_location(rhs), + closure: region.closure, + compound_statement: region.compound_statement, + conditional: true, + node: rhs, + source: :parser + ) + pins.push rhs_cs + NodeProcessor.process(rhs, region.update(compound_statement: rhs_cs), pins, locals, ivars) FlowSensitiveTyping.new(locals, ivars, diff --git a/lib/solargraph/parser/parser_gem/node_processors/orasgn_node.rb b/lib/solargraph/parser/parser_gem/node_processors/orasgn_node.rb index 17480adfb5..3326391c4a 100644 --- a/lib/solargraph/parser/parser_gem/node_processors/orasgn_node.rb +++ b/lib/solargraph/parser/parser_gem/node_processors/orasgn_node.rb @@ -8,7 +8,19 @@ class OrasgnNode < Parser::NodeProcessor::Base # @return [void] def process new_node = node.updated(node.children[0].type, node.children[0].children + [node.children[1]]) - NodeProcessor.process(new_node, region, pins, locals, ivars) + # `x ||= y` only assigns when x is falsy/undefined, so + # it's never a guaranteed override of x's prior type + # + asgn_cs = Solargraph::Pin::CompoundStatement.new( + location: get_node_location(node), + closure: region.closure, + compound_statement: region.compound_statement, + conditional: true, + node: node, + source: :parser + ) + pins.push asgn_cs + NodeProcessor.process(new_node, region.update(compound_statement: asgn_cs), pins, locals, ivars) end end end diff --git a/lib/solargraph/parser/parser_gem/node_processors/resbody_node.rb b/lib/solargraph/parser/parser_gem/node_processors/resbody_node.rb index 24846748fb..35f0143159 100644 --- a/lib/solargraph/parser/parser_gem/node_processors/resbody_node.rb +++ b/lib/solargraph/parser/parser_gem/node_processors/resbody_node.rb @@ -30,7 +30,17 @@ def process source: :parser ) end - NodeProcessor.process(node.children[2], region, pins, locals, ivars) + rescue_body_node = node.children[2] + rescue_body_cs = Solargraph::Pin::CompoundStatement.new( + location: rescue_body_node ? get_node_location(rescue_body_node) : nil, + closure: region.closure, + compound_statement: region.compound_statement, + conditional: true, + node: rescue_body_node, + source: :parser + ) + pins.push rescue_body_cs + NodeProcessor.process(rescue_body_node, region.update(compound_statement: rescue_body_cs), pins, locals, ivars) if rescue_body_node end end end diff --git a/lib/solargraph/parser/parser_gem/node_processors/until_node.rb b/lib/solargraph/parser/parser_gem/node_processors/until_node.rb index 2e091f41d1..f345e00953 100644 --- a/lib/solargraph/parser/parser_gem/node_processors/until_node.rb +++ b/lib/solargraph/parser/parser_gem/node_processors/until_node.rb @@ -13,14 +13,17 @@ def process # until statement doesn't create a closure - e.g., # variables created inside can be seen from outside as # well - pins.push Solargraph::Pin::Until.new( + until_pin = Solargraph::Pin::Until.new( location: location, closure: region.closure, + compound_statement: region.compound_statement, + conditional: true, node: node, comments: comments_for(node), source: :parser ) - process_children region + pins.push until_pin + process_children region.update(compound_statement: until_pin) end end end diff --git a/lib/solargraph/parser/parser_gem/node_processors/when_node.rb b/lib/solargraph/parser/parser_gem/node_processors/when_node.rb index 915eb57e65..144220d48c 100644 --- a/lib/solargraph/parser/parser_gem/node_processors/when_node.rb +++ b/lib/solargraph/parser/parser_gem/node_processors/when_node.rb @@ -8,13 +8,16 @@ class WhenNode < Parser::NodeProcessor::Base include ParserGem::NodeMethods def process - pins.push Solargraph::Pin::CompoundStatement.new( + cs = Solargraph::Pin::CompoundStatement.new( location: get_node_location(node), closure: region.closure, + compound_statement: region.compound_statement, + conditional: true, node: node, source: :parser ) - process_children + pins.push cs + process_children region.update(compound_statement: cs) end end end diff --git a/lib/solargraph/parser/parser_gem/node_processors/while_node.rb b/lib/solargraph/parser/parser_gem/node_processors/while_node.rb index 6c4fe33d86..44b30f84eb 100644 --- a/lib/solargraph/parser/parser_gem/node_processors/while_node.rb +++ b/lib/solargraph/parser/parser_gem/node_processors/while_node.rb @@ -17,14 +17,17 @@ def process # while statement doesn't create a closure - e.g., # variables created inside can be seen from outside as # well - pins.push Solargraph::Pin::While.new( + while_pin = Solargraph::Pin::While.new( location: location, closure: region.closure, + compound_statement: region.compound_statement, + conditional: true, node: node, comments: comments_for(node), source: :parser ) - process_children region + pins.push while_pin + process_children region.update(compound_statement: while_pin) end end end diff --git a/lib/solargraph/parser/region.rb b/lib/solargraph/parser/region.rb index 8c4caf6acb..d535a36840 100644 --- a/lib/solargraph/parser/region.rb +++ b/lib/solargraph/parser/region.rb @@ -21,15 +21,29 @@ class Region # @return [Array] attr_reader :lvars + # The nearest enclosing CompoundStatement pin (an if/when/while/ + # rescue/&&/||/||= body, a method/block body, or a namespace + # body) - a series of statements/expressions where a later one + # executing implies the earlier ones in the same series + # executed too. Every Closure is also a CompoundStatement, so + # this is a superset of the `closure` chain: it additionally + # includes branch bodies that aren't scopes. + # + # @return [Pin::CompoundStatement] + attr_reader :compound_statement + # @param source [Source] # @param closure [Pin::Closure, nil] # @param scope [Symbol, nil] # @param visibility [Symbol] # @param lvars [Array] + # @param compound_statement [Pin::CompoundStatement, nil] def initialize source: Solargraph::Source.load_string(''), closure: nil, - scope: nil, visibility: :public, lvars: [] + scope: nil, visibility: :public, lvars: [], + compound_statement: nil @source = source @closure = closure || Pin::Namespace.new(name: '', location: source.location, source: :parser) + @compound_statement = compound_statement || @closure @scope = scope @visibility = visibility @lvars = lvars @@ -54,14 +68,17 @@ def namespace_pin # @param scope [Symbol, nil] # @param visibility [Symbol, nil] # @param lvars [Array, nil] + # @param compound_statement [Pin::CompoundStatement, nil] # @return [Region] - def update closure: nil, scope: nil, visibility: nil, lvars: nil + def update closure: nil, scope: nil, visibility: nil, lvars: nil, + compound_statement: nil Region.new( source: source, closure: closure || self.closure, scope: scope || self.scope, visibility: visibility || self.visibility, - lvars: lvars || self.lvars + lvars: lvars || self.lvars, + compound_statement: compound_statement || self.compound_statement ) end diff --git a/lib/solargraph/pin/base_variable.rb b/lib/solargraph/pin/base_variable.rb index c7945e5998..f27322813b 100644 --- a/lib/solargraph/pin/base_variable.rb +++ b/lib/solargraph/pin/base_variable.rb @@ -14,6 +14,17 @@ class BaseVariable < Base # @return [Range, nil] attr_reader :presence + # @return [Boolean] + attr_reader :definite + + # The CompoundStatement pin this variable's (re)assignment was + # made within - i.e. Region#compound_statement at the point of + # assignment. Used by #definite_reaches? to decide whether a + # non-definite assignment still dominates a given reference. + # + # @return [Pin::CompoundStatement, nil] + attr_reader :compound_statement + # @param return_type [ComplexType, nil] # @param assignment [Parser::AST::Node, nil] First assignment # that was made to this variable @@ -45,10 +56,25 @@ class BaseVariable < Base # @see https://www.typescriptlang.org/docs/handbook/2/everyday-types.html#union-types # @see https://en.wikipedia.org/wiki/Intersection_type#TypeScript_example # @param presence [Range, nil] + # @param definite [Boolean] True if this pin's assignment(s) are + # guaranteed to have executed at (and after) its presence's + # start position, as opposed to being inside a conditional + # branch or loop that may not run. Used to decide whether a + # reassignment's type may safely override a variable's + # previously declared/inferred type instead of merely being + # unioned with it. + # @param compound_statement [Pin::CompoundStatement, nil] The + # CompoundStatement this variable's (re)assignment was made + # within. When `definite` is false, a reference whose location + # falls within this pin's own range may still treat the + # assignment as an override rather than merely unioning it + # with earlier possible types - see #definite_reaches?. # @param [Hash{Symbol => Object}] splat def initialize assignment: nil, assignments: [], mass_assignment: nil, presence: nil, return_type: nil, intersection_return_type: nil, exclude_return_type: nil, + definite: true, + compound_statement: nil, **splat super(**splat) @assignments = (assignment.nil? ? [] : [assignment]) + assignments @@ -58,6 +84,8 @@ def initialize assignment: nil, assignments: [], mass_assignment: nil, @intersection_return_type = intersection_return_type @exclude_return_type = exclude_return_type @presence = presence + @definite = definite + @compound_statement = compound_statement end # @param presence [Range] @@ -82,20 +110,55 @@ def reset_generated! super end - def combine_with other, attrs = {} - new_assignments = combine_assignments(other) + # @param other [self] + # @param attrs [Hash] + # @param location [Location, nil] The position being resolved, + # if known - used to decide whether a not-globally-definite + # `other` should still override us because the position falls + # within `other`'s compound_statement. + def combine_with other, attrs = {}, location: nil + superseded = override_assignments?(other, location) + # Facts expire only when a *different* assignment overwrote the + # value they describe. Two flow-sensitive downcasts of the same + # assignment - e.g. the one per operand that `a.nil? || + # a.empty? || a == 'x'` produces - are additional facts about + # one value, and must accumulate rather than replace each other. + facts_superseded = superseded && !same_assignment_sites?(other) + new_assignments = combine_assignments(other, location) new_attrs = attrs.merge({ # default values don't exist in RBS parameters; it just # tells you if the arg is optional or not. Prefer a # provided value if we have one here since we can't rely on # it from RBS so we can infer from it and typecheck on it. - assignment: choose(other, :assignment), + # + # When #combine_assignments supersedes rather than unions, + # skip this - the constructor prepends `assignment:` to + # `assignments:` unconditionally, which would re-introduce + # the dropped node. + assignment: superseded ? nil : choose(other, :assignment), assignments: new_assignments, mass_assignment: combine_mass_assignment(other), return_type: combine_return_type(other), - intersection_return_type: combine_types(other, :intersection_return_type), - exclude_return_type: combine_types(other, :exclude_return_type), - presence: combine_presence(other) + # Narrowing recorded against the old value expires + # when that value is definitely overwritten, so when + # `other`'s assignment supersedes ours, keep only the + # facts asserted about the new value. + intersection_return_type: if facts_superseded + other.intersection_return_type + else + combine_types(other, :intersection_return_type) + end, + exclude_return_type: if facts_superseded + other.exclude_return_type + else + combine_types(other, :exclude_return_type) + end, + presence: combine_presence(other), + # if either side had an assignment guaranteed to + # have executed, that assignment's type is + # eligible to override (not just be unioned + # with) the variable's other possible types + definite: definite || other.definite || superseded }) super(other, new_attrs) end @@ -109,6 +172,22 @@ def combine_mass_assignment other mass_assignment || other.mass_assignment end + # True when `other`'s assignments are the very same ones as ours, + # identified by source position. Structural node equality is not + # usable here - two textually identical assignments on different + # lines compare equal, and telling those apart is the whole point. + # + # @param other [self] + # @return [Boolean] + def same_assignment_sites? other + assignment_sites == other.assignment_sites + end + + # @return [::Array] + def assignment_sites + assignments.map { |node| Solargraph::Range.from_node(node) } + end + # @return [Parser::AST::Node, nil] def assignment @assignment ||= assignments.last @@ -116,8 +195,12 @@ def assignment # @param other [self] # + # @param other [self] + # @param location [Location, nil] # @return [::Array] - def combine_assignments other + def combine_assignments other, location = nil + return other.assignments.dup if override_assignments?(other, location) + (other.assignments + assignments).uniq end @@ -283,6 +366,7 @@ def visible_at? other_closure, other_loc location.filename == other_loc.filename && # @sg-ignore flow sensitive typing needs to handle attrs (!presence || presence.include?(other_loc.range.start)) && + !within_own_assignment?(other_loc) && visible_in_closure?(other_closure) end @@ -293,8 +377,120 @@ def visible_at? other_closure, other_loc # @return [Range] attr_writer :presence + public + + # True if `other_loc` falls inside the source range of one of this + # pin's own assignment value nodes - i.e., `other_loc` is + # resolving a reference that occurs *while* one of this + # variable's own assignments is still being evaluated, such as + # the receiver `x` in a self-referential reassignment (`x = + # x.length`, or `index += 1` desugared to `index = index + 1`). + # That reference must resolve against this variable's *other* + # assignments, not against the not-yet-assigned value being + # derived here, even though `other_loc` otherwise falls within + # this pin's presence. + # + # @param other_loc [Location] + # @return [Boolean] + def within_own_assignment? other_loc + return false unless location&.filename == other_loc.filename + + assignments.any? do |assignment_node| + next false unless assignment_node.respond_to?(:loc) + + rng = Range.from_node(assignment_node) + next false if rng.nil? + + # The position immediately at/after the assignment node's own + # end is where its new value becomes visible - only exclude + # positions strictly *inside* the node (i.e. still being + # evaluated), not that boundary itself. + rng.contain?(other_loc.range.start) && other_loc.range.start != rng.ending + end + end + + private + + # True if `other`'s assignment(s) should supersede ours + # instead of merely being unioned with them: `other` reassigns + # the same variable, in the same scope, via an assignment + # guaranteed to have executed, so by the time `other`'s + # presence begins our value has definitely been overwritten. + # + # Excludes self-referential reassignments (`x = x.foo`, + # desugared `+=`, etc.) - resolving their right-hand side needs + # our assignment(s) as the base case, so dropping them would + # leave nothing to resolve against. + # + # @param other [self] + # @param location [Location, nil] The position being resolved, + # if known - lets a conditional `other` still override us when + # `location` falls within `other`'s compound_statement. + # @return [Boolean] + def override_assignments? other, location = nil + (other.definite || other.definite_reaches?(location)) && other.closure == closure && + other.assignments.none? { |node| references_name?(node) } + end + + public + + # True if this pin's assignment, though not globally definite, + # is still guaranteed to dominate `location` - i.e., `location` + # falls within the CompoundStatement body (an if/while/until/ + # rescue/&&/||/||= branch) this assignment was made in, so no + # earlier branch exit could have skipped it by the time + # `location` is reached. A nested CompoundStatement's location + # is always a subrange of its parent's, so this single + # containment check already accounts for arbitrarily nested + # branches without walking the compound_statement chain further. + # + # @param location [Location, nil] + # @return [Boolean] + def definite_reaches? location + cs = compound_statement + return false unless location && cs&.location&.range + + # @sg-ignore flow sensitive typing needs to handle attrs + cs.location.filename == location.filename && + # @sg-ignore flow sensitive typing needs to handle attrs + cs.location.range.contain?(location.range.start) + end + private + # @param node [Parser::AST::Node, nil] + # @return [Boolean] + def references_name? node + return false unless node.is_a?(::AST::Node) + + # @sg-ignore flow sensitive typing doesn't narrow `node` past the guard above + return true if %i[lvar ivar].include?(node.type) && node.children[0].to_s == name + + # A block parameter of the same name shadows us for the whole + # block, so any mention inside the body is the parameter, not + # this variable. The receiver (children[0]) is evaluated + # outside the block, so it still counts. + # @sg-ignore flow sensitive typing doesn't narrow `node` past the guard above + return references_name?(node.children[0]) if shadowed_by_block_parameter?(node) + + # @sg-ignore flow sensitive typing doesn't narrow `node` past the guard above + node.children.any? { |child| references_name?(child) } + end + + # @param node [::AST::Node] + # @return [Boolean] + def shadowed_by_block_parameter? node + return false unless node.type == :block + + args = node.children[1] + return false unless args.is_a?(::AST::Node) + + # @sg-ignore flow sensitive typing doesn't narrow `args` past the guard above + args.children.any? do |arg| + arg.is_a?(::AST::Node) && arg.children[0].to_s == name + end + end + # @param api_map [ApiMap] # @param raw_return_type [ComplexType, ComplexType::UniqueType] # diff --git a/lib/solargraph/pin/compound_statement.rb b/lib/solargraph/pin/compound_statement.rb index 39d9cf2d5c..c527d6928a 100644 --- a/lib/solargraph/pin/compound_statement.rb +++ b/lib/solargraph/pin/compound_statement.rb @@ -44,11 +44,71 @@ module Pin class CompoundStatement < Pin::Base attr_reader :node + # The immediately enclosing CompoundStatement, if any - nil only + # for the synthetic root Namespace Region creates for top-level + # code. Since Closure < CompoundStatement, walking this chain + # until an ancestor is_a?(Closure) is how Base#closure is + # derived when a pin has no directly-assigned @closure. + # + # @return [Pin::CompoundStatement, nil] + attr_reader :compound_statement + + # True if this construct's body may be skipped, or run zero or + # multiple times, at runtime - e.g. an if/while/until/rescue/&&/ + # ||/||= body, or a block body (which, despite being a Closure + # like Method/Namespace, may run zero or many times depending on + # the method it's passed to, unlike a method/namespace body, + # which always runs exactly once when reached). Defaults false - + # true only where a node processor explicitly marks a construct + # as conditionally executed. + # + # @return [Boolean] + attr_reader :conditional + # @param node [Parser::AST::Node, nil] + # @param compound_statement [Pin::CompoundStatement, nil] + # @param conditional [Boolean] # @param [Hash{Symbol => Object}] splat - def initialize node: nil, **splat + def initialize node: nil, compound_statement: nil, conditional: false, **splat super(**splat) @node = node + @compound_statement = compound_statement + @conditional = conditional + end + + # @param other [self] + # @param attrs [Hash{Symbol => Object}] + # @return [self] + def combine_with other, attrs = {} + new_attrs = { + compound_statement: combine_compound_statement(other), + conditional: choose(other, :conditional) + }.merge(attrs) + super(other, new_attrs) + end + + # Bare CompoundStatement pins (if/when/rescue/&&/||/||= bodies) + # all share name == '', so the same-name-assertion in + # Base#choose_pin_attr_with_same_name (used by #combine_closure) + # would be meaningless noise here - pick by location instead, + # mirroring BaseVariable#combine_closure. + # + # @param other [self] + # @return [Pin::CompoundStatement, nil] + def combine_compound_statement other + return compound_statement if compound_statement == other.compound_statement + return compound_statement || other.compound_statement if compound_statement.nil? || other.compound_statement.nil? + + # @sg-ignore flow sensitive typing needs to handle attrs + if compound_statement.location.nil? || other.compound_statement.location.nil? + # @sg-ignore flow sensitive typing needs to handle attrs + return compound_statement.location.nil? ? other.compound_statement : compound_statement + end + + # @sg-ignore flow sensitive typing needs to handle attrs + return compound_statement if compound_statement.location <= other.compound_statement.location + + other.compound_statement end end end diff --git a/lib/solargraph/pin/local_variable.rb b/lib/solargraph/pin/local_variable.rb index 077da21be3..e73ba2588c 100644 --- a/lib/solargraph/pin/local_variable.rb +++ b/lib/solargraph/pin/local_variable.rb @@ -16,9 +16,9 @@ def probe api_map super end - def combine_with other, attrs = {} + def combine_with other, attrs = {}, location: nil # keep this as a parameter - return other.combine_with(self, attrs) if other.is_a?(Parameter) && !is_a?(Parameter) + return other.combine_with(self, attrs, location: location) if other.is_a?(Parameter) && !is_a?(Parameter) super end diff --git a/lib/solargraph/pin/method.rb b/lib/solargraph/pin/method.rb index 81cc94d284..1e39b45203 100644 --- a/lib/solargraph/pin/method.rb +++ b/lib/solargraph/pin/method.rb @@ -668,14 +668,14 @@ def infer_from_return_nodes api_map end rng = Range.from_node(n) next unless rng - clip = api_map.clip_at( - # @sg-ignore Need to add nil check here - location.filename, - rng.ending - ) + # A flow-sensitive downcast's presence can end before the return + # node's own end (e.g. inside `!(foo.nil? || foo < 5)`); chain + # resolution re-checks each local's presence at its own sub-node + # location, so pass the full local set rather than pre-filtering here. # @sg-ignore Need to add nil check here - chain = Solargraph::Parser.chain(n, location.filename) - type = chain.infer(api_map, self, clip.locals) + all_locals = api_map.source_map(filename).locals + chain = Solargraph::Parser.chain(n, filename) + type = chain.infer(api_map, self, all_locals) result.push type unless type.undefined? end result.push ComplexType::NIL if has_nil diff --git a/lib/solargraph/pin/parameter.rb b/lib/solargraph/pin/parameter.rb index ba20976ec6..39f49ce8df 100644 --- a/lib/solargraph/pin/parameter.rb +++ b/lib/solargraph/pin/parameter.rb @@ -30,7 +30,7 @@ def location super || closure&.type_location end - def combine_with other, attrs = {} + def combine_with other, attrs = {}, location: nil # Parameters can only be combined with local variables in the same closure return self unless other.closure == closure @@ -45,7 +45,7 @@ def combine_with other, attrs = {} asgn_code: asgn_code } end - super(other, new_attrs.merge(attrs)) + super(other, new_attrs.merge(attrs), location: location) end def combine_return_type other @@ -208,6 +208,13 @@ def index # @param api_map [ApiMap] def typify api_map + if definite + # Reassigned by an assignment guaranteed to have executed: + # prefer that value's type over the declared parameter type. + reassigned_type = probe(api_map) + return reassigned_type if reassigned_type.defined? + end + new_type = super return new_type if new_type.defined? diff --git a/lib/solargraph/position.rb b/lib/solargraph/position.rb index 11d8eb8d5c..1a4dbcd78f 100644 --- a/lib/solargraph/position.rb +++ b/lib/solargraph/position.rb @@ -68,8 +68,6 @@ def self.to_offset text, position end last_line_index += 1 if position.line.positive? - # @sg-ignore `last_line_index` is always an Integer because `newline_index` - # is never nil inside the while block last_line_index + position.character end @@ -99,16 +97,12 @@ def self.from_offset text, offset character = offset newline_index = -1 - # @sg-ignore Typechecker thinks `newline_index` inside of the assignment - # can be nil while (newline_index = text.index("\n", newline_index + 1)) && newline_index < offset line += 1 - # @sg-ignore `newline_index` is always an Integer inside the while block character = offset - newline_index - 1 end character = 0 if character.nil? && (cursor - offset).between?(0, 1) raise InvalidOffsetError if character.nil? - # @sg-ignore flow sensitive typing needs to handle 'raise if' Position.new(line, character) end diff --git a/lib/solargraph/range.rb b/lib/solargraph/range.rb index e1ed895921..e7f49e0344 100644 --- a/lib/solargraph/range.rb +++ b/lib/solargraph/range.rb @@ -46,11 +46,8 @@ def to_hash # @return [Boolean] def contain? position position = Position.normalize(position) - # @sg-ignore flow sensitive typing should be able to handle redefinition return false if position.line < start.line || position.line > ending.line - # @sg-ignore flow sensitive typing should be able to handle redefinition return false if position.line == start.line && position.character < start.character - # @sg-ignore flow sensitive typing should be able to handle redefinition return false if position.line == ending.line && position.character > ending.character true end @@ -58,11 +55,9 @@ def contain? position # True if the range contains the specified position and the position does not precede it. # # @param position [Position, Array(Integer, Integer)] - # @sg-ignore flow sensitive typing should be able to handle redefinition # @return [Boolean] def include? position position = Position.normalize(position) - # @sg-ignore flow sensitive typing should be able to handle redefinition contain?(position) && !(position.line == start.line && position.character == start.character) end diff --git a/lib/solargraph/source/cursor.rb b/lib/solargraph/source/cursor.rb index 077364910a..147b03b6cb 100644 --- a/lib/solargraph/source/cursor.rb +++ b/lib/solargraph/source/cursor.rb @@ -54,7 +54,6 @@ def start_of_word # `foo.bar`, the end_of_word at position (0,6) is `r`. # # @return [String] - # @sg-ignore Need to add nil check here def end_of_word @end_of_word ||= begin match = source.code[offset..].to_s.match(end_word_pattern) diff --git a/lib/solargraph/source/source_chainer.rb b/lib/solargraph/source/source_chainer.rb index f96fa3319e..f8a778a57f 100644 --- a/lib/solargraph/source/source_chainer.rb +++ b/lib/solargraph/source/source_chainer.rb @@ -118,7 +118,6 @@ def fixed_position end # @return [String] - # @sg-ignore Need to add nil check here def end_of_phrase @end_of_phrase ||= begin match = phrase.match(/\s*(\.{1}|::)\s*$/) diff --git a/lib/solargraph/workspace/gemspecs.rb b/lib/solargraph/workspace/gemspecs.rb index 2c29b948c6..756203a88b 100644 --- a/lib/solargraph/workspace/gemspecs.rb +++ b/lib/solargraph/workspace/gemspecs.rb @@ -63,7 +63,6 @@ def resolve_require require begin gemspec = Gem::Specification.find_by_name(gem_name) - # @sg-ignore flow sensitive typing should be able to handle redefinition return [gemspec_or_preference(gemspec)] if gemspec rescue Gem::MissingSpecError logger.debug do @@ -106,7 +105,6 @@ def find_gem name, version = nil, out: $stderr # @sg-ignore flow sensitive typing should be able to handle redefinition specish = all_gemspecs_from_bundle.find { |specish| specish.name == name } - # @sg-ignore flow sensitive typing needs to create separate ranges for postfix if return to_gem_specification specish if specish resolve_gem_ignoring_local_bundle name, version, out: out diff --git a/spec/parser/flow_sensitive_typing_spec.rb b/spec/parser/flow_sensitive_typing_spec.rb index 4c9034873b..4e6701a456 100644 --- a/spec/parser/flow_sensitive_typing_spec.rb +++ b/spec/parser/flow_sensitive_typing_spec.rb @@ -314,6 +314,22 @@ def baz; end expect(clip.infer.to_s).to eq('Foo') end + it 'keeps a definite reassignment visible inside a subsequent if-guard' do + source = Solargraph::Source.load_string(%( + def m + x = nil + x = 1 + if x + y = x * 2 + end + end + ), 'test.rb') + + api_map = Solargraph::ApiMap.new.map(source) + clip = api_map.clip_at('test.rb', [5, 14]) + expect(clip.infer.rooted_tags).to eq('::Integer') + end + it 'skips is_a? without a receiver' do source = Solargraph::Source.load_string(%( if is_a? Object @@ -899,6 +915,30 @@ def bar(baz: nil) expect(clip.infer.rooted_tags).to eq('::Boolean') end + it 'does not extend a ||= body guard to a variable other than the assignment target' do + source = Solargraph::Source.load_string(%( + class Foo + # @param baz [::Boolean, nil] + # @param qux [::Boolean, nil] + # @return [void] + def bar(baz: nil, qux: nil) + baz ||= begin + return if qux.nil? + qux + end + qux + end + end + ), 'test.rb') + + api_map = Solargraph::ApiMap.new.map(source) + + # skipping the body means baz was truthy, which says nothing + # about qux, so the guard does not survive the ||= + clip = api_map.clip_at('test.rb', [10, 10]) + expect(clip.infer.rooted_tags).to eq('::Boolean, nil') + end + it 'uses .nil? in a return if() in a try / rescue / ensure to refine types using nil checks' do source = Solargraph::Source.load_string(%( class Foo diff --git a/spec/pin/compound_statement_spec.rb b/spec/pin/compound_statement_spec.rb new file mode 100644 index 0000000000..d46b908a52 --- /dev/null +++ b/spec/pin/compound_statement_spec.rb @@ -0,0 +1,89 @@ +# frozen_string_literal: true + +describe Solargraph::Pin::CompoundStatement do + # Pins are threaded with both an explicit `closure:` and a + # `compound_statement:` chain. These specs walk the chain and check + # it reaches the same closure, catching a node processor that + # threads one without the other. + def derive_closure pin + cs = pin.compound_statement + cs = cs.compound_statement while cs && !cs.is_a?(Solargraph::Pin::Closure) + cs + end + + it 'agrees with the stored closure for compound statements nested in a method, if, and while' do + source_map = Solargraph::SourceMap.load_string(%( + class Foo + def bar(flag) + if flag + while flag + local = 1 + end + end + end + end + )) + + compound_statement_pins = source_map.pins.select { |pin| pin.is_a?(described_class) } + expect(compound_statement_pins).not_to be_empty + + compound_statement_pins.each do |pin| + expect(derive_closure(pin)).to eq(pin.closure), "mismatch for #{pin.inspect}" + end + end + + it 'agrees with the stored closure for compound statements nested in a block' do + source_map = Solargraph::SourceMap.load_string(%( + class Foo + def bar + [1].each do |i| + if i + local = i + end + end + end + end + )) + + compound_statement_pins = source_map.pins.select { |pin| pin.is_a?(described_class) } + expect(compound_statement_pins).not_to be_empty + + compound_statement_pins.each do |pin| + expect(derive_closure(pin)).to eq(pin.closure), "mismatch for #{pin.inspect}" + end + end + + describe '#combine_with' do + let(:earlier_location) { Solargraph::Location.new('test.rb', Solargraph::Range.from_to(1, 0, 3, 0)) } + let(:later_location) { Solargraph::Location.new('test.rb', Solargraph::Range.from_to(5, 0, 7, 0)) } + + it 'prefers the compound_statement with the earlier location' do + earlier_cs = described_class.new(location: earlier_location, source: :parser) + later_cs = described_class.new(location: later_location, source: :parser) + pin1 = described_class.new(location: earlier_location, compound_statement: earlier_cs, source: :parser) + pin2 = described_class.new(location: later_location, compound_statement: later_cs, source: :parser) + + expect(pin1.combine_with(pin2).compound_statement).to eq(earlier_cs) + expect(pin2.combine_with(pin1).compound_statement).to eq(earlier_cs) + end + + it 'prefers the compound_statement with a location when the other has none' do + cs_no_location = described_class.new(location: nil, source: :parser) + cs_with_location = described_class.new(location: later_location, source: :parser) + pin1 = described_class.new(location: earlier_location, compound_statement: cs_no_location, source: :parser) + pin2 = described_class.new(location: earlier_location, compound_statement: cs_with_location, source: :parser) + + expect(pin1.combine_with(pin2).compound_statement).to eq(cs_with_location) + expect(pin2.combine_with(pin1).compound_statement).to eq(cs_with_location) + end + + it 'prefers a non-nil compound_statement over a nil one' do + cs = described_class.new(location: earlier_location, source: :parser) + pin1 = described_class.new(location: earlier_location, compound_statement: nil, source: :parser) + pin2 = described_class.new(location: earlier_location, compound_statement: cs, source: :parser) + + expect(pin1.combine_with(pin2).compound_statement).to eq(cs) + expect(pin2.combine_with(pin1).compound_statement).to eq(cs) + end + end +end diff --git a/spec/source/chain_spec.rb b/spec/source/chain_spec.rb index a6b29686e9..3b50d4942c 100644 --- a/spec/source/chain_spec.rb +++ b/spec/source/chain_spec.rb @@ -363,8 +363,6 @@ class Bar; end end it 'infers instance variables from sequential assignments' do - pending('sequential assignment support') - source = Solargraph::Source.load_string(%( def foo @foo = nil diff --git a/spec/source_map/clip_spec.rb b/spec/source_map/clip_spec.rb index 468612ab1c..1952d3bf8c 100644 --- a/spec/source_map/clip_spec.rb +++ b/spec/source_map/clip_spec.rb @@ -2441,8 +2441,6 @@ def bar; end end it 'replaces nil with reassignments' do - pending 'sequential assignment support' - source = Solargraph::Source.load_string(%( bar = nil bar @@ -2458,8 +2456,6 @@ def bar; end end it 'replaces type with reassignments' do - pending 'sequential assignment support' - source = Solargraph::Source.load_string(%( bar = 'a' bar diff --git a/spec/type_checker/levels/strong_spec.rb b/spec/type_checker/levels/strong_spec.rb index 9a0bd3e8a4..ef45b915b6 100644 --- a/spec/type_checker/levels/strong_spec.rb +++ b/spec/type_checker/levels/strong_spec.rb @@ -954,6 +954,536 @@ def maybe_bar? expect(checker.problems.map(&:message)).to eq([]) end + it 'updates a parameter type after reassignment to a different non-literal type' do + checker = type_checker(%( + class Position + # @return [Integer] + def line + 1 + end + end + + module PositionNormalizer + # @param position [Position, Array(Integer, Integer)] + # @return [Position] + def self.normalize(position) + Position.new + end + end + + # @param position [Position, Array(Integer, Integer)] + # @return [Integer] + def describe(position) + position = PositionNormalizer.normalize(position) + position.line + end + )) + expect(checker.problems.map(&:message)).to eq([]) + end + + it 'does not treat a parameter reassignment inside a block as guaranteed to have run' do + checker = type_checker(%( + class Position + # @return [Integer] + def line + 1 + end + end + + module PositionNormalizer + # @param position [Position, Array(Integer, Integer)] + # @return [Position] + def self.normalize(position) + Position.new + end + end + + # @param position [Position, Array(Integer, Integer)] + # @return [Integer] + def describe(position) + [1].each { position = PositionNormalizer.normalize(position) } + position.line + end + )) + expect(checker.problems.map(&:message)).to eq([ + '#describe return type could not be inferred', + 'Unresolved call to line on Position, Array(Integer, Integer)' + ]) + end + + it 'still treats a conditional reassignment as guaranteed to have run for a use site inside the same branch' do + checker = type_checker(%( + # @param str [String] + # @param num [Integer] + # @param flag [Boolean] + # @return [void] + def conditional_reassign(str, num, flag) + local = num + if flag + local = str + local.upcase + end + end + )) + expect(checker.problems.map(&:message)).to eq([]) + end + + it 'narrows a variable assigned in the if condition' do + checker = type_checker(%( + # @param name [String] + # @return [Integer] + def limit_of(name) + if (md = name.match(/\\[(.*)\\]/)) + md[1].to_i + else + 0 + end + end + )) + expect(checker.problems.map(&:message)).to eq([]) + end + + it 'narrows a variable assigned in the right side of an && condition' do + checker = type_checker(%( + # @param name [String, nil] + # @return [Integer] + def limit_of(name) + if !name.nil? && (md = name.match(/\\[(.*)\\]/)) + md[1].to_i + else + 0 + end + end + )) + expect(checker.problems.map(&:message)).to eq([]) + end + + it 'does not narrow a variable assigned in the left side of an || condition' do + checker = type_checker(%( + # @param name [String] + # @param fallback [Boolean] + # @return [Integer] + def limit_of(name, fallback) + if (md = name.match(/\\[(.*)\\]/)) || fallback + md[1].to_i + else + 0 + end + end + )) + expect(checker.problems.map(&:message)).to eq(['Unresolved call to []']) + end + + it 'treats a variable assigned in the if condition as falsy in the else clause' do + checker = type_checker(%( + # @param name [String] + # @return [Integer] + def limit_of(name) + if (md = name.match(/\\[(.*)\\]/)) + 0 + else + md[1].to_i + end + end + )) + # the falsy-only receiver renders as either `nil, false` or + # `nil, Boolean` depending on literal handling; both mean narrowed + expect(checker.problems.map(&:message)) + .to contain_exactly(a_string_matching(/\AUnresolved call to \[\] on nil, (false|Boolean)\z/)) + end + + it 'uses a branch-local reassignment at a use site later in the same branch' do + checker = type_checker(%( + # @param items [Array, nil] + # @return [void] + def clean(items) + if items.nil? + items = fetch_items + items.reject! { |i| i.empty? } + end + end + + # @return [Array] + def fetch_items; ['x']; end + )) + expect(checker.problems.map(&:message)).to eq([]) + end + + it 'does not use a reassignment made in a nested branch that may not run' do + checker = type_checker(%( + # @param items [Array, nil] + # @param flag [Boolean] + # @return [void] + def clean(items, flag) + if items.nil? + if flag + items = fetch_items + end + items.reject! { |i| i.empty? } + end + end + + # @return [Array] + def fetch_items; ['x']; end + )) + expect(checker.problems.map(&:message)).to eq(['Unresolved call to reject! on nil']) + end + + it 'does not use a branch-local reassignment at a use site before it' do + checker = type_checker(%( + # @param items [Array, nil] + # @return [void] + def clean(items) + if items.nil? + items.reject! { |i| i.empty? } + items = fetch_items + end + end + + # @return [Array] + def fetch_items; ['x']; end + )) + expect(checker.problems.map(&:message)).to eq(['Unresolved call to reject! on nil']) + end + + it 'accumulates every fact an or-guard asserts about the same value' do + checker = type_checker(%( + # @param name [String, Integer, nil] + # @return [String] + def f(name) + a = lookup(name) + a = 'd' if a.nil? || a.is_a?(Integer) + a + end + + # @param name [String, Integer, nil] + # @return [String, Integer, nil] + def lookup(name); end + )) + expect(checker.problems.map(&:message)).to eq([]) + end + + it 'narrows a nil-guarded default behind an or-guard with three operands' do + checker = type_checker(%( + # @param name [String, nil] + # @return [String] + def f(name) + a = lookup(name) + a = 'd' if a.nil? || a.empty? || a == 'x' + a + end + + # @param name [String, nil] + # @return [String, nil] + def lookup(name); end + )) + expect(checker.problems.map(&:message)).to eq([]) + end + + it 'narrows a nil-guarded default behind an or-guard with four operands' do + checker = type_checker(%( + # @param name [String, nil] + # @return [String] + def f(name) + a = lookup(name) + a = 'd' if a.nil? || a.empty? || a == 'x' || a == 'y' + a + end + + # @param name [String, nil] + # @return [String, nil] + def lookup(name); end + )) + expect(checker.problems.map(&:message)).to eq([]) + end + + # Soundness controls for the or-guard narrowing above. `¬(x || y)` implies + # every operand is false, so the guard's false path may narrow any variable + # it tests - but the guard's TRUE path only reassigns `a`, so nothing may be + # concluded about a second variable the guard happens to mention. + it 'does not narrow a second variable an or-guard tests but never reassigns' do + checker = type_checker(%( + # @param name [String, nil] + # @return [String] + def f(name) + a = lookup(name) + b = lookup(name) + a = 'd' if a.nil? || b.nil? + b + end + + # @param name [String, nil] + # @return [String, nil] + def lookup(name); end + )) + expect(checker.problems.map(&:message)) + .to include(a_string_matching(/Declared return type ::String does not match/)) + end + + it 'does not narrow when the or-guard never tests the variable at all' do + checker = type_checker(%( + # @param name [String, nil] + # @param flag [Boolean] + # @return [String] + def f(name, flag) + a = lookup(name) + a = 'd' if flag || name.nil? + a + end + + # @param name [String, nil] + # @return [String, nil] + def lookup(name); end + )) + expect(checker.problems.map(&:message)) + .to include(a_string_matching(/Declared return type ::String does not match/)) + end + + it 'applies a modifier-if guard after the variable was reassigned' do + checker = type_checker(%( + # @param name [String] + # @return [Integer, nil] + def find(name) + got = lookup(name) + return got.length if got + + got = lookup(name) + got.length if got + end + + # @param name [String] + # @return [String, nil] + def lookup(name); name; end + )) + expect(checker.problems.map(&:message)).to eq([]) + end + + it 'applies a modifier-if guard after a reassignment whose block shadows the name' do + checker = type_checker(%( + # @param name [String] + # @return [Integer, nil] + def find(name) + got = candidates.find { |got| got == name } + return got.length if got + + got = candidates.find { |got| got != name } + got.length if got + end + + # @return [Array] + def candidates; []; end + )) + expect(checker.problems.map(&:message)).to eq([]) + end + + it 'keeps a guard fact in force until the variable is reassigned' do + checker = type_checker(%( + # @param name [String] + # @return [Integer, nil] + def find(name) + got = lookup(name) + return got.length if got + + got.length + end + + # @param name [String] + # @return [String, nil] + def lookup(name); name; end + )) + expect(checker.problems.map(&:message)) + .to contain_exactly(a_string_matching(/\AUnresolved call to length on nil, (false|Boolean)\z/)) + end + + it 'does not apply a guard fact past a reassignment that only runs in a branch' do + checker = type_checker(%( + # @param name [String] + # @param flag [Boolean] + # @return [Integer, nil] + def find(name, flag) + got = lookup(name) + return got.length if got + + if flag + got = lookup(name) + end + got.length + end + + # @param name [String] + # @return [String, nil] + def lookup(name); name; end + )) + expect(checker.problems.map(&:message)) + .to contain_exactly(a_string_matching(/\AUnresolved call to length on nil, (false|Boolean)\z/)) + end + + it 'narrows a nil-guarded default after the modifier if' do + checker = type_checker(%( + # @param tasks [Array, nil] + # @return [void] + def guarded_default(tasks) + tasks = ['a'] if tasks.nil? + tasks.each { |t| puts t } + end + )) + expect(checker.problems.map(&:message)).to eq([]) + end + + it 'narrows a nil-guarded default after a non-modifier if' do + checker = type_checker(%( + # @param tasks [Array, nil] + # @return [void] + def guarded_default(tasks) + if tasks.nil? + tasks = ['a'] + end + tasks.each { |t| puts t } + end + )) + expect(checker.problems.map(&:message)).to eq([]) + end + + it 'narrows a guarded default assigned in an unless modifier' do + checker = type_checker(%( + # @param tasks [Array, nil] + # @return [void] + def guarded_default(tasks) + tasks = ['a'] unless tasks + tasks.each { |t| puts t } + end + )) + expect(checker.problems.map(&:message)).to eq([]) + end + + it 'narrows a guarded default assigned in an else clause' do + checker = type_checker(%( + # @param tasks [Array, nil] + # @return [void] + def guarded_default(tasks) + if !tasks.nil? + puts 'have tasks' + else + tasks = ['a'] + end + tasks.each { |t| puts t } + end + )) + expect(checker.problems.map(&:message)).to eq([]) + end + + it 'narrows only the reassigned variable when an or-condition guards it' do + checker = type_checker(%( + # @param xs [Array, nil] + # @param ys [Array, nil] + # @return [void] + def or_guard(xs, ys) + xs = ['a'] if xs.nil? || ys.nil? + xs.each { |t| puts t } + ys.each { |t| puts t } + end + )) + expect(checker.problems.map(&:message)).to eq(['Unresolved call to each on Array, nil']) + end + + it 'keeps nil in the type when the guard tests something other than the variable' do + checker = type_checker(%( + # @param tasks [Array, nil] + # @param flag [Boolean] + # @return [void] + def unrelated_guard(tasks, flag) + tasks = ['a'] if flag + tasks.each { |t| puts t } + end + )) + expect(checker.problems.map(&:message)).to eq(['Unresolved call to each on Array, nil']) + end + + it 'keeps nil in the type when the nil guard does not reassign the variable' do + checker = type_checker(%( + # @param tasks [Array, nil] + # @return [void] + def no_reassignment(tasks) + puts 'hi' if tasks.nil? + tasks.each { |t| puts t } + end + )) + expect(checker.problems.map(&:message)).to eq(['Unresolved call to each on Array, nil']) + end + + it 'keeps nil in the type when the guarded assignment is itself conditional' do + checker = type_checker(%( + # @param tasks [Array, nil] + # @param flag [Boolean] + # @return [void] + def nested_conditional_assign(tasks, flag) + if tasks.nil? + tasks = ['a'] if flag + end + tasks.each { |t| puts t } + end + )) + expect(checker.problems.map(&:message)).to eq(['Unresolved call to each on Array, nil']) + end + + it 'does not let a loop-body reassignment override a reference textually before it' do + checker = type_checker(%( + # @param str [String] + # @param num [Integer] + # @param flag [Boolean] + # @return [void] + def loop_reassign(str, num, flag) + local = num + while flag + local.abs + local = str + end + end + )) + expect(checker.problems.map(&:message)).to eq([]) + end + + it 'updates a local variable type after reassignment to a different literal type' do + checker = type_checker(%( + # @return [void] + def run + local = 5 + local = 'hello' + local.upcase + nil + end + )) + expect(checker.problems.map(&:message)).to eq([]) + end + + it 'updates an instance variable type after reassignment in the same method' do + checker = type_checker(%( + class Foo + # @return [void] + def run + @ivar = 5 + @ivar = 'hello' + @ivar.upcase + nil + end + end + )) + expect(checker.problems.map(&:message)).to eq([]) + end + + it 'resolves a self-referential reassignment against the pre-assignment type' do + checker = type_checker(%( + class Repro + # @param x [String] + # @return [Integer] + def foo(x) + x = x.length + end + end + )) + expect(checker.problems.map(&:message)).to eq([]) + end + it 'supports !@x.nil && @x.y' do checker = type_checker(%( class Bar @@ -970,6 +1500,17 @@ def foo? expect(checker.problems.map(&:message)).to eq([]) end + it 'infers a Boolean return from !!(x.nil? || x < n) on a nilable param' do + checker = type_checker(%( + # @param val [Integer, nil] + # @return [Boolean] + def check?(val) + !!(val.nil? || val < 5) + end + )) + expect(checker.problems.map(&:message)).to eq([]) + end + it 'uses cast type instead of defined type' do checker = type_checker(%( # frozen_string_literal: true