Skip to content
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

Links to symbol definitions are incorrect. #57

Closed
mattchamb opened this issue Dec 19, 2014 · 8 comments · Fixed by #61
Closed

Links to symbol definitions are incorrect. #57

mattchamb opened this issue Dec 19, 2014 · 8 comments · Fixed by #61
Assignees
Labels

Comments

@mattchamb
Copy link

I click on the "parent" argument variable on this line:
http://sourcebrowser.io/Browse/CodeConnect/SourceBrowser/SourceBrowser.Generator/DocumentWalker.cs#33

and it takes me here:
http://sourcebrowser.io/Browse/CodeConnect/SourceBrowser/SourceBrowser.Generator/Transformers/AbstractWorkspaceVisitor.cs#35

instead of to the line where the argument is declared. Similar things happen for links on other symbols too.

@JoshVarty
Copy link
Contributor

Sorry for the delay.

That's really weird. I'll take a look at this now.

@JoshVarty JoshVarty self-assigned this Dec 22, 2014
@mattchamb
Copy link
Author

All good, no rush on my end. Just noticed this while checking out what you guys had done

@AmadeusW
Copy link
Member

I think the issue happens in CSWalker.cs
at "localLink.ReferencedSymbolName = symbol.ToString();" in ProcessSymbolUsage

The symbol we're getting is SourceBrowser.Generator.Model.IProjectItem and we know that symbol.Kind is Parameter. Is it possible that the current model doesn't support links to parameters? When we're building the link, we're only assigning the literal name "SourceBrowser.Generator.Model.IProjectItem" to localLink.ReferencedSymbolName. I think the issue is that this information is not enough to accurately point to the parameter definition.

btw, I'm catching the token in question in VisitToken:

        if (token.Value?.ToString() == "parent")
        {
            int test = 3; // set breakpoint here
        }

@JoshVarty
Copy link
Contributor

It's a combination of two bugs.

  1. We improperly handle parameters. We're giving them the fully qualified name of their type. (ie. SourceBrowser.Generator.Model.IProjectItem) We should be linking them to the method for which they are a parameter.
  2. We assume there's only one definition for each type when building links. (We've been ignoring partial types and classes). This means that all 'IProjectItem' parameters in the entire projection will point to a single definition. In this case it's http://sourcebrowser.io/Browse/CodeConnect/SourceBrowser/SourceBrowser.Generator/Transformers/AbstractWorkspaceVisitor.cs#35

I'll fix 1, which will fix this.

The second will require some changes in both the Generator and the Site. I'll add an issue for that if one doesn't exist already.

@AmadeusW
Copy link
Member

In case 2 it doesn't actually point to definition of IProjectItem. It points to one of the methods that accepts that type as parameter. Is it a fluke, or an incorrect implementation of case 1?

@JoshVarty
Copy link
Contributor

It's because every method that accepts IProjectItem is incorrectly saying that IProjectIem is defined there. But it has to choose just one, so it looks like happened to choose this method. In theory it could have randomly chosen the correct definition of IProjectItem.

@AmadeusW
Copy link
Member

Alright. So we need to compare the fully qualified name of the token's parent method with names of methods that say that define IProjectItem?

@JoshVarty
Copy link
Contributor

We're not handling the declaration of parameters correctly. They are getting a fully qualified name identical to their type. So in

public void Test(JQuerySelector selector) 
{
}

When we process selector, we're erroneously giving it the fully qualified name "Selenium.WebDriver.Extensions.JQuerySelector" which means anything that links to JQuerySelector might link here.

This should be an easy fix, thankfully.

@JoshVarty JoshVarty reopened this Dec 27, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants