Skip to content

fix: Add support for unresolved fields from mixins #116

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 18 commits into from
Oct 4, 2022

Conversation

varungandhi-src
Copy link
Contributor

@varungandhi-src varungandhi-src commented Sep 21, 2022

Motivation

Mixins are used very frequently. We should have better code nav for them.

TODO

  • Manually test Go to Def and Find References when an undeclared field is present in different files. (Currently, this will create two different SymbolInformation values for the same symbol in different files.)

  • Manually test an inheritance hierarchy involving multiple classes. In that situation, it seems like the transitive linkage should be happening through relation traversal. E.g. if C < B < A then C.@f would be defined by B.@f which would be defined by A.@f. In that case, we should check if all references to A.@f show up when we do Find references on C.@f. There is a TODO related to this, which should be updated/removed.

  • Remove debugging statements

  • Right now, we emit a SymbolInformation is emitted when a definition is emitted, but it is possible for a field inside a class coming from a mixin to not have any definition inside the class. Consider:

    module M
        def set_f_0
            @f = 0
        end
    end
    class C
        def set_f_1
            @f = 1
        end
    end
    class D < C
        include M
        def get_f
            @f
        end
    end

    The code is currently set up so that:

    1. The reference inside a class will emit a symbol for the further ancestor which mentions the class. (However, strictly speaking, we don't require a definition inside that ancestor's body right now... -- but it would be strange for a superclass to access a field that it never sets, so...). In this case, the occurrence for @f in get_f will use C#@f. (← Not sure if this is a good idea though, see next point.)
    2. We emit a reference relationship for symbols that are mentioned (transitively) in mixins when emitting the definition. In this case, there is no definition for @f inside D, so no relationship is emitted. (Additionally, it is not guaranteed that we can modify C#@f's SymbolInformation, since C may be coming from a different index altogether. -- This makes me question if we should be instead using D@#f for the occurrence of @f instead.)

    It is unclear to me what exactly we should do. There are a couple of options:

    1. Emit a fake definition for D#@f (what location should be used?), and attach reference relationships to it which connect it to M#@f and C#@f.
    2. Emit 2 occurrences for every @f in D, one for C#@f and another for M#@f.
  • More tests, including for complex include chains

Test plan

See included automated tests.

@varungandhi-src varungandhi-src force-pushed the vg/mixins branch 2 times, most recently from 7fa5c45 to 0ba671e Compare September 26, 2022 13:11
@varungandhi-src varungandhi-src changed the title [WIP] fix: Add support for unresolved fields from mixins fix: Add support for unresolved fields from mixins Sep 26, 2022
Comment on lines +107 to +114
if (sym.exists()) { // TODO(varun): Is this early exit justified?
// Maybe it is possible to hit this in multiple ancestors?
Copy link
Contributor Author

@varungandhi-src varungandhi-src Sep 27, 2022

Choose a reason for hiding this comment

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

Follow-up: Test this in a running Sourcegraph instance with an inheritance hierarchy involving multiple classes. In that situation, it seems like the transitive linkage should be happening through relation traversal. E.g. if C < B < A then C.@f would be defined by B.@f which would be defined by A.@f. In that case, we should check if all references to A.@f show up when we do Find references on C.@f.

If that does work, then remove this TODO with a brief explanation.

This makes it consistent with other SCIPState caches.
There is a risk of higher memory usage but it removes
the risk of confusion/incorrect navigation in the presence
of unrelated identically-named classes in different files.
@varungandhi-src
Copy link
Contributor Author

Something seems to be off with the ref panel, preventing manual testing. Let's merge this first... I'll file a follow-up issue for manual testing. https://github.com/sourcegraph/sourcegraph/issues/42499

@varungandhi-src varungandhi-src merged commit 819c885 into scip-ruby/master Oct 4, 2022
@varungandhi-src varungandhi-src deleted the vg/mixins branch October 4, 2022 11:41
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.

1 participant