Skip to content

Provide resolution diagnostics#474

Open
Morriar wants to merge 9 commits intomainfrom
at-resolver-diagnostics
Open

Provide resolution diagnostics#474
Morriar wants to merge 9 commits intomainfrom
at-resolver-diagnostics

Conversation

@Morriar
Copy link
Contributor

@Morriar Morriar commented Jan 15, 2026

Please review #472 and #473 first.

Closes #435.

Introduce 6 new diagnostics:

  • KindRedefinition: redefining a class as a module or vice versa
  • ParentRedefinition: redefining the parent of a class
  • NonClassSuperclass: subclassing something that is not a class
  • CircularDependency: introducing a mixin or parent class cycle
  • NonModuleMixin: including something that is not a module
  • UnresolvedConstantReference: using a constant that can't be resolved

@Morriar Morriar requested a review from a team as a code owner January 15, 2026 23:03
@Morriar Morriar force-pushed the at-resolver-diagnostics branch 3 times, most recently from d1158c6 to 065dc23 Compare January 19, 2026 19:52
fn validate_constant_references(&mut self) {
let mut diagnostics = Vec::new();

for reference in self.graph.constant_references().values() {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of looping through the entire list, I think we can just add diagnostics directly to anything that's still in the unit_queue, no?

Whatever is left is what we failed to resolve.

diagnostics
}

fn validate_mixins(&self, declaration: &Declaration) -> Vec<Diagnostic> {
Copy link
Member

Choose a reason for hiding this comment

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

For both of the mixin diagnostics, are we not able to add them during linearization to avoid looping afterwards?

for definition in declaration.definitions().iter().skip(1) {
let definition = self.graph.definitions().get(definition).unwrap();
let definition_declaration_kind = DeclarationKind::from_definition_kind(definition.kind());
if definition_declaration_kind != first_definition_declaration_kind {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we can just compare that they're not the same. For example, this is valid code:

Foo = Struct.new

class Foo
  include Bar
end

I was looking into this specific case as part of the struct work and I believe we need these checks (or maybe some of them) in Graph#add_declaration, so that we can auto-promote certain declaration cases like the one in this example.

Another valid case is having a method declaration that has two definitions: one method and one attribute.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The trick is in from_definition_kind where we map the output definition kind to declaration kind:

   pub fn from_definition_kind(definition_kind: DefinitionKind) -> Self {
        match definition_kind {
            DefinitionKind::Class => DeclarationKind::Class,
            DefinitionKind::SingletonClass => DeclarationKind::SingletonClass,
            DefinitionKind::Module => DeclarationKind::Module,
            DefinitionKind::Constant => DeclarationKind::Constant,
            DefinitionKind::ConstantAlias => DeclarationKind::ConstantAlias,
            DefinitionKind::Method
            | DefinitionKind::MethodAlias
            | DefinitionKind::AttrAccessor
            | DefinitionKind::AttrReader
            | DefinitionKind::AttrWriter => DeclarationKind::Method,
            DefinitionKind::InstanceVariable => DeclarationKind::InstanceVariable,
            DefinitionKind::ClassVariable => DeclarationKind::ClassVariable,
            DefinitionKind::GlobalVariable | DefinitionKind::GlobalVariableAlias => DeclarationKind::GlobalVariable,

So both an attr accessor and a method definition map to a method declaration so this is correct:

def foo; end
attr_reader :foo

For Struct, this would be the same:

 match definition_kind {
            DefinitionKind::Class | DefinitionKind::Struct => DeclarationKind::Class,

@Morriar Morriar self-assigned this Jan 19, 2026
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
@Morriar Morriar force-pushed the at-resolver-diagnostics branch from 065dc23 to 1e92a1c Compare January 21, 2026 18:48
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
@Morriar
Copy link
Contributor Author

Morriar commented Jan 21, 2026

@vinistock I pushed another PR #502 to show what it would look like to compute the diagnostics during the resolution rather than after.

TL;DR: it's not faster but more invasive and complex.

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