Add resolution diagnostic for declaration kind mismatch

[Morriar]

One more step towards #354.

This required a way to assert on the kind of the declaration. I added a DeclarationKind enum than can be used to both build a new declaration (which removes the need for closures in the resolution) and know what kind an existing declaration is.

Another subtlety is to which definition location to attach the error. Because declarations are created in a specific order that may not match their order in the code, we need to add some logic so the error is displayed on the last introduced one rather than the first one.

This PR is easier to review commit by commit.

[st0012]

This is not used?

[Morriar]

Introduced too early, I can remove it from here 👍

[st0012]

Does this mean, if a library of my project has

# a fixture file bundled in the gem
# not required by my project
class User; end

And in my project, I define

module User
  class Something; end
end

I'll then get an error always show up in Ruby LSP?

[Morriar]

Yes, which should be the same with Sorbet until you ignore the fixtures directory.

[st0012]

But unlike Sorbet, rubydex under the usage of Ruby LSP will index dependencies, regardless whether they'd be required or not (which is different then gem rbi Tapioca generates through runtime). So in this case the chance of having this error will be much higher. And when that happens, users will need to exclude specific gem file/folder, which is harder to configure.
I think we should be more conservative on showing this as errors. IMO warning would be better.

[vinistock]

My 2 cents: I think this should be an error. We only index gems' require paths. If a gem includes a fixture as part of its require path with code that conflicts with the real gem's implementation, I would argue that's a mistake in the gem.

Re-defining a class/module type crashes in the runtime, so I would stick with error.

[st0012]

Oh I didn't know we only index the gem's require path. Where is this implemented?
If that's the case, then I'm good with this.

[vinistock]

This exact line

rubydex/lib/rubydex/graph.rb

Line 34 in c8c27f8

spec.require_paths.map { |path| File.join(spec.full_gem_path, path) }

[st0012]

What about cases like the recent RDoc patch? If we introduce Struct as a new type of definition, wouldn't that mean RDoc would always trigger this error?

[vinistock]

There are two parts to the answer here. For Struct.new, it returns a class. We just need special handling for it, which I'm working on #392.

The second aspect I realized is that we actually need to be able to promote declarations since we can't control what kind of meta-programming a gem or project might be using. For example:

# Create a class via meta-programming. Rubydex has no idea and thinks this is
# a constant
Foo = meta_programming_class do
  # ....
end

# Then re-open the class and perform operations that are only valid in a namespace
# context, like including a module
class Foo
  include Bar
end

At the moment, we would be crashing in this case, so I have a branch where I'm exploring automatic promotion of declarations. Basically, if we find something that was a constant and then gets re-opened as a class, the most correct behaviour might be to promote it.

However, we can't allow redefinition from a class to a module. That's always an error.

[Morriar]

@st0012 raised interesting questions and I think we can do better, please take a look at #450.

[Morriar]

I rebased this work using the rules from #450.

[vinistock]

Considering we can have definitions across files that we cannot know the load order, does sorting really make a difference?

I'd say we can pick one to add the diagnostic and then we use related information to associate the diagnostic with all other definitions.

[vinistock]

This refactor will conflict with your other refactor idea that requires enqueueing an ancestor linearization unit only when the closure to produce a new declaration gets invoked.

It might be worth splitting this refactor and considering it as part of the other one.

[Morriar]

Closing in favor of #474.