-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Add hasLocationInfo
for Type
s
#12131
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
Conversation
The changes look fine, but it looks like they have made some of the tests fail and they might need updating as well. |
go/ql/lib/semmle/go/Types.qll
Outdated
dt.getType() = this and | ||
// Note that if the type declaration isn't in the source that we have | ||
// then we use a dummy location. | ||
dt.hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to explicitly return the dummy location when there isn't such a DeclaredType
It returns a dummy location except for named types with a type declaration in the source.
Note that go/ql/test/library-tests/semmle/go/Types/QualifiedNames.expected and go/ql/test/library-tests/semmle/go/Types/Types.expected gain two lines. In both cases this is because GenericArray and GenericSignature are each instantiated twice, so they appear with two different locations.
03704a8
to
685b8b4
Compare
The original version was giving multiple locations for some types because it was giving the locations of type aliases as well. The refactoring seems to fix that, which is good as we don't want to get duplicate results. |
DCA it; otherwise lgtm |
@smowton The first DCA failed for unknown reasons, but the second seems absolutely uneventful, as I'd expect. |
It returns a dummy location except for named types with a type declaration in the source.