Some signatures are broken due to lack of module scoping about sord

[AaronC81]

I'm not sure - the output from Sorbet looks a bit fancier than what I'm used to seeing from the stdlib, off the top of my head. (I suppose they could still be using did_you_mean behind the scenes though.) I'll look into it tomorrow.

…

On Sun, 23 Jun 2019, 02:07 Connor Shea, ***@***.***> wrote: Do they literally just use the did_you_mean gem from the stdlib? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#25?email_source=notifications&email_token=AAZK7LN547ZL47AAFKLCILLP33EFFA5CNFSM4H2XYIUKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYKUJAY#issuecomment-504710275>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZK7LLQPHJNEXNBUBEOOITP33EFFANCNFSM4H2XYIUA> .

[connorshea]

This is 87% of the type errors in discordrb now :)

[AaronC81]

@connorshea Give the (highly experimental!) nested-namespaces branch a go.

No beautification yet, you might want to run rufo <whatever>.rbi after generation if you'd like to read the RBI. (gem install rufo if you don't already have it.)

If I'm testing it correctly, this reduces discordrb's errors from around 900 to just 160!

The logical next step is some system which attempts to resolve constant names which aren't in the same namespace. It could be an [INFER] if it's unambiguous and there's only one class with that name in the codebase, or [WARN ] if there are multiple options.

[connorshea]

Yup! srb tc --counters:

Before:

Errors: 871
Counters and Histograms: 
 error
                           Total :       871
                            4010 :        14,   1.6% #
                            4012 :         8,   0.9% 
                            5002 :       762,  87.5% #############################################################
                            5036 :         1,   0.1% 
                            5037 :        29,   3.3% ##
                            7002 :         2,   0.2% 
                            7003 :         9,   1.0% 
                            7004 :         2,   0.2% 
                            7005 :        41,   4.7% ###
                            7006 :         3,   0.3% 

After:

Errors: 153
Counters and Histograms: 
 error
                           Total :       153
                            4010 :        14,   9.2% ######
                            4012 :         8,   5.2% ###
                            5002 :        33,  21.6% ###############
                            5036 :         1,   0.7% 
                            5037 :        29,  19.0% #############
                            7002 :         2,   1.3% 
                            7003 :         9,   5.9% ####
                            7004 :         2,   1.3% 
                            7005 :        52,  34.0% #######################
                            7006 :         3,   2.0% #

[AaronC81]

Thinking about ways that this, from my earlier comment, could be implemented...

The logical next step is some system which attempts to resolve constant names which aren't in the same namespace. It could be an [INFER] if it's unambiguous and there's only one class with that name in the codebase, or [WARN ] if there are multiple options.

Sorbet's "did you mean" messages seem to have the correct suggestion pretty much 100% of the time. I wonder if these can be accessed programmatically easily?

[connorshea]

Do they literally just use the did_you_mean gem from the stdlib?

[AaronC81]

Looking into this: while the did_you_mean gem is referenced in Sorbet, it looks like they're using their own fuzzy-matcher system for "did you mean"s of constants (https://github.com/sorbet/sorbet/blob/df6085e6ae9846f033064513ea14f069c68b0bf9/resolver/resolver.cc#L263).

So, if we wanted to use Sorbet's system, there are two ways of doing it that I can think of:

1. Run Sorbet and parse the output, then replace the constants in the generated RBI with Sorbet's suggestions. Unless there's a "machine-readable output" command-line switch on the srb tool yet, this really doesn't seem very stable.

2. Link against Sorbet's C extensions and "dry-run" Sorbet across the RBI file, acting on the suggestions. This would probably be pretty hard (if even possible) to implement, but it would likely be more robust than parsing srb output.

Alternatively, we could create an entirely different system that doesn't use Sorbet's "did you mean"s. I'm not sure whether I'm overthinking this!

[AaronC81]

Actually, it looks like the "machine-readable output" I might be after could be in Sorbet's LSP implementation. I'll experiment with this!

[connorshea]

I don't think there's really an existing LSP client implementation in Ruby, so this'll be interesting.

[AaronC81]

I started implementing this on the "lsp_resolver" branch, but it's not being particularly easy to work with. I don't know whether this is an issue with my LSP client implementation or Sorbet's LSP server implementation.

For example, if I load up Discordrb, it doesn't appear to be sending the entire collection of diagnostics; it sends two chunks then stops sending anything.

I'll put the LSP idea on hold for a bit and maybe look at parsing Sorbet's output instead.

[connorshea]

I think for now adding some code that matches the type if it unambiguously matches only one constant would be the easiest big improvement

e.g. with VoiceBot here:

./rbi/discordrb.rbi:4663: Unable to resolve constant VoiceBot https://srb.help/5002
    4663 |      sig { returns(T::Hash[Integer, VoiceBot]) }
                                               ^^^^^^^^
    ./lib/discordrb/voice/voice_bot.rb:23: Did you mean: Discordrb::Voice::VoiceBot?
    23 |  class VoiceBot

[AaronC81]

@connorshea - I've implemented this using YARD's registry and it seems to work pretty well! Have an experiment with the yard-resolution branch if you like.

It will replace the unresolvable class, logging an [INFER], if there is exactly one candidate for replacement. If it can't find a replacement, or there is more than one, then it logs a [WARN ] instead.

Running srb on the RBI generated by the yard-resolution branch for Discordrb results in only 13 errors!

[connorshea]

👏

[AaronC81]

Merged in eee7429!

[connorshea]

There are some new warnings caused by the implementation, like IO being 'unresolvable', or objects that come from other libraries like RestClient::Response being unresolvable. Maybe instead of 'x was unresolvable' it should say something like 'x wasn't able to be resolved to a constant in this project'?

Also, since IO is a core Ruby class I wonder if we could have a list of core Ruby classes to prevent them from being reused by Sord, e.g. if you also had a class called Library::Module::IO in your library and the YARD doc just said IO, it should be considered ambiguous.

[AaronC81]

Yeah, I'd agree that wording is better.

Regarding the second point, it does actually check for core classes:

sord/lib/sord/resolver.rb

Lines 20 to 28 in 2fe81b4

	def self.builtin_classes
	Object.constants
	.map(&:to_s)
	.select { |x| /[a-z]/ === x }
	end
	def self.resolvable?(name, item, include_builtins = true)
	# Check if it's a builtin
	return true if include_builtins && builtin_classes.include?(name)

There's a bug introduced because it checks whether the elements in Object.constants are classes, rather than a built-in constant like RUBY_VERSION, by seeing if they contain any lowercase characters. Obviously IO is a class but doesn't actually contain lowercase characters, so is rejected. (This is why other core types like String, Integer, etc currently work fine.) I'll change this check to something more robust.

Also, core types currently take precedence over any YARD class, which probably isn't perfect for ambiguity as you've pointed out.

[AaronC81]

I've pushed changes to the yard-resolution branch which should make built-ins work as described here. I'd still like to add some tests before merging.

[AaronC81]

Tests added, so this is merged!