Skip to content

Fix bug of unexpectedly resolving for an absolute class#156

Merged
soutaro merged 1 commit intoruby:masterfrom
pocke:absolute
Jan 2, 2020
Merged

Fix bug of unexpectedly resolving for an absolute class#156
soutaro merged 1 commit intoruby:masterfrom
pocke:absolute

Conversation

@pocke
Copy link
Member

@pocke pocke commented Dec 31, 2019

Problem

Ruby signature resolves unknown type unexpectedly if the type is absolute. That means it has :: prefix.

require 'ruby/signature'

include Ruby::Signature

# Env without signature
env = Environment.new()


c1 = Parser.parse_type('C')
c2 = Parser.parse_type('::C')
namespace = Namespace.new(path: [], absolute: true)

p env.absolute_type_name(c1.name, namespace: namespace){} # => nil
p env.absolute_type_name(c2.name, namespace: namespace){} # => It print c2.name

I expect both type names, which are c1 and c2, are resolved as nil. Because the empty environment has no classes.
But actually c2 is resolved as itself.

Problem in the real application

I found the bug while I'm trying to use Steep.
I got different messages between C and ::C in superclass.

# sig1.rbs
class A < C
end

# sig2.rbs
class A < ::C
end

First, I got the following output with sig1.rbs. I think it is the expected output.

$ steep check
sigtest/sig1.rbs:2:0...3:3       UnknownTypeNameError: name=::C

However, I got an error with sig2.rbs.

$ steep check
#<NoMethodError: undefined method `location' for nil:NilClass>
  /home/pocke/ghq/github.com/ruby/ruby-signature/lib/ruby/signature/definition_builder.rb:20:in `build_ancestors'
  /home/pocke/ghq/github.com/ruby/ruby-signature/lib/ruby/signature/definition_builder.rb:47:in `build_ancestors'
  /home/pocke/ghq/github.com/ruby/ruby-signature/lib/ruby/signature/definition_builder.rb:211:in `block in build_instance'
  /home/pocke/ghq/github.com/ruby/ruby-signature/lib/ruby/signature/definition_builder.rb:811:in `try_cache'
  /home/pocke/ghq/github.com/ruby/ruby-signature/lib/ruby/signature/definition_builder.rb:203:in `build_instance'
  /home/pocke/ghq/github.com/soutaro/steep/lib/steep/signature/validator.rb:55:in `block in validate_one_decl'
  /home/pocke/ghq/github.com/soutaro/steep/lib/steep/signature/validator.rb:106:in `rescue_validation_errors'
  /home/pocke/ghq/github.com/soutaro/steep/lib/steep/signature/validator.rb:53:in `validate_one_decl'
  /home/pocke/ghq/github.com/soutaro/steep/lib/steep/signature/validator.rb:74:in `block in validate_decl'
  /home/pocke/ghq/github.com/ruby/ruby-signature/lib/ruby/signature/environment.rb:71:in `block in each_decl'
  /home/pocke/ghq/github.com/ruby/ruby-signature/lib/ruby/signature/environment.rb:70:in `each'
  /home/pocke/ghq/github.com/ruby/ruby-signature/lib/ruby/signature/environment.rb:70:in `each_decl'
  /home/pocke/ghq/github.com/soutaro/steep/lib/steep/signature/validator.rb:73:in `validate_decl'
  /home/pocke/ghq/github.com/soutaro/steep/lib/steep/signature/validator.rb:45:in `validate'
  /home/pocke/ghq/github.com/soutaro/steep/lib/steep/project/target.rb:136:in `load_signatures'
  /home/pocke/ghq/github.com/soutaro/steep/lib/steep/project/target.rb:86:in `type_check'
  /home/pocke/ghq/github.com/soutaro/steep/lib/steep/drivers/utils/driver_helper.rb:76:in `block (2 levels) in type_check'
  /home/pocke/.rbenv/versions/2.6.4/lib/ruby/gems/2.6.0/gems/activesupport-5.2.4.1/lib/active_support/tagged_logging.rb:71:in `block in tagged'
  /home/pocke/.rbenv/versions/2.6.4/lib/ruby/gems/2.6.0/gems/activesupport-5.2.4.1/lib/active_support/tagged_logging.rb:28:in `tagged'
  /home/pocke/.rbenv/versions/2.6.4/lib/ruby/gems/2.6.0/gems/activesupport-5.2.4.1/lib/active_support/tagged_logging.rb:71:in `tagged'
  /home/pocke/ghq/github.com/soutaro/steep/lib/steep/drivers/utils/driver_helper.rb:75:in `block in type_check'
  /home/pocke/ghq/github.com/soutaro/steep/lib/steep/drivers/utils/driver_helper.rb:74:in `each'
  /home/pocke/ghq/github.com/soutaro/steep/lib/steep/drivers/utils/driver_helper.rb:74:in `type_check'
  /home/pocke/ghq/github.com/soutaro/steep/lib/steep/drivers/check.rb:25:in `run'
  /home/pocke/ghq/github.com/soutaro/steep/lib/steep/cli.rb:91:in `process_check'
  /home/pocke/ghq/github.com/soutaro/steep/lib/steep/cli.rb:50:in `run'
  /home/pocke/ghq/github.com/soutaro/steep/exe/steep:12:in `<top (required)>'
  /home/pocke/.rbenv/versions/2.6.4/lib/ruby/gems/2.6.0/bin/steep:23:in `load'
  /home/pocke/.rbenv/versions/2.6.4/lib/ruby/gems/2.6.0/bin/steep:23:in `<top (required)>'
  /home/pocke/.rbenv/versions/2.6.4/lib/ruby/2.6.0/bundler/cli/exec.rb:74:in `load'
  /home/pocke/.rbenv/versions/2.6.4/lib/ruby/2.6.0/bundler/cli/exec.rb:74:in `kernel_load'
  /home/pocke/.rbenv/versions/2.6.4/lib/ruby/2.6.0/bundler/cli/exec.rb:28:in `run'
  /home/pocke/.rbenv/versions/2.6.4/lib/ruby/2.6.0/bundler/cli.rb:463:in `exec'
  /home/pocke/.rbenv/versions/2.6.4/lib/ruby/2.6.0/bundler/vendor/thor/lib/thor/command.rb:27:in `run'
  /home/pocke/.rbenv/versions/2.6.4/lib/ruby/2.6.0/bundler/vendor/thor/lib/thor/invocation.rb:126:in `invoke_command'
  /home/pocke/.rbenv/versions/2.6.4/lib/ruby/2.6.0/bundler/vendor/thor/lib/thor.rb:387:in `dispatch'
  /home/pocke/.rbenv/versions/2.6.4/lib/ruby/2.6.0/bundler/cli.rb:27:in `dispatch'
  /home/pocke/.rbenv/versions/2.6.4/lib/ruby/2.6.0/bundler/vendor/thor/lib/thor/base.rb:466:in `start'
  /home/pocke/.rbenv/versions/2.6.4/lib/ruby/2.6.0/bundler/cli.rb:18:in `start'
  /home/pocke/.rbenv/versions/2.6.4/lib/ruby/gems/2.6.0/gems/bundler-1.17.2/exe/bundle:30:in `block in <top (required)>'
  /home/pocke/.rbenv/versions/2.6.4/lib/ruby/2.6.0/bundler/friendly_errors.rb:124:in `with_friendly_errors'
  /home/pocke/.rbenv/versions/2.6.4/lib/ruby/gems/2.6.0/gems/bundler-1.17.2/exe/bundle:22:in `<top (required)>'
  /home/pocke/.rbenv/versions/2.6.4/bin/bundle:23:in `load'
  /home/pocke/.rbenv/versions/2.6.4/bin/bundle:23:in `<main>'

Steep will print the same output with both signatures by this patch.

SignatureManager.new do |manager|
manager.files[Pathname("foo.rbs")] = <<EOF
class Array[Elem]
end
Copy link
Member Author

@pocke pocke Dec 31, 2019

Choose a reason for hiding this comment

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

Array is not defined by SignatureManager in test_helper.
So the test failed by this patch. Because ::Array on the next line cannot be resolved.

I considered adding Array definition to the SignatureManager, but other tests define Array class, and they will conflict with SignatureManager's one if I define Array.
So I decided to add it here.

@soutaro soutaro merged commit 32a8177 into ruby:master Jan 2, 2020
@soutaro
Copy link
Member

soutaro commented Jan 2, 2020

Great fix! @pocke 👏

@pocke pocke deleted the absolute branch January 2, 2020 05:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants