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

Go: Add tests for model inheritance and fix bug in promoted methods #17505

Merged
merged 10 commits into from
Sep 26, 2024

Conversation

owen-mc
Copy link
Contributor

@owen-mc owen-mc commented Sep 18, 2024

These tests show several things:

  1. That the subtypes column of MaD has no effect for non-interface methods.
  2. That the common use of getACall() in QL models is overly broad: see from the ql_S1 comments that by modeling S1 we also match calls to I1, which is an interface that S1 implements. This is not what users would expect.
  3. Models for promoted methods act as if they are actually models for the method on the type that it was originally defined on. This is not what users would expect.

This PR also fixes a bug where methods of an embedded field were promoted when they shouldn't have been (because the type already has a method of that name).

@owen-mc owen-mc added the no-change-note-required This PR does not need a change note label Sep 18, 2024
@owen-mc owen-mc requested a review from a team as a code owner September 18, 2024 11:29
@github-actions github-actions bot added the Go label Sep 18, 2024
pack: codeql/go-all
extensible: sourceModel
data:
- ["github.com/nonexistent/test", "I1", False, "Source", "", "", "ReturnValue", "remote", "manual"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting to note that with subtypes = false we actually do model a struct that embeds I1 here. I think we shouldn't (not to fix here, a note for the future)

@@ -0,0 +1,40 @@
invalidModelRow
paths
| test.go:8:7:8:16 | call to Source | test.go:10:9:10:9 | y |
Copy link
Contributor

Choose a reason for hiding this comment

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

(Probably good to note these in the corresponding QL files in the manner we would with $ MISSING and similar if this was an inline-expectations test): I believe these are all good results except for the cases that shadow the relevant methods which are FPs.

@@ -0,0 +1,10 @@
invalidModelRow
paths
| test.go:14:7:14:16 | call to Source | test.go:16:9:16:9 | y |
Copy link
Contributor

Choose a reason for hiding this comment

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

Again noting expectations to cross-check: I think the embedding result is unexpected

@@ -0,0 +1,22 @@
invalidModelRow
paths
| test.go:14:7:14:16 | call to Source | test.go:16:9:16:9 | y |
Copy link
Contributor

Choose a reason for hiding this comment

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

And these are all good except for the overriding cases which are FPs

@@ -0,0 +1,10 @@
invalidModelRow
paths
| test.go:20:7:20:16 | call to Source | test.go:22:9:22:9 | y |
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to I1, the embedding result is bad

@@ -0,0 +1,10 @@
invalidModelRow
paths
| test.go:20:7:20:16 | call to Source | test.go:22:9:22:9 | y |
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently subtypes = true for a non-interface does nothing; if it did anything in the future it would be to include embeddings, which we do here by accident

t.Sink(y)
}

func TestStructEmbeddingAndOverridingS2(t test.StructEmbeddingAndOverridingS2) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest one more case to include: interface-in-interface embedding, which is a slightly different beast to X-in-struct embedding, where X may be an interface or a struct, since it doesn't create a field

@owen-mc
Copy link
Contributor Author

owen-mc commented Sep 24, 2024

@smowton I've revamped the tests to be inline expectations tests. It's just so much easier to deal with. I've added the case you suggested. I've fixed a bug that was causing us to promote methods that shouldn't have been promoted because the receiver type already has a method with the same name. Even with this bug fixed, I agree that the current behaviour of our MaD tests around promoted methods should be changed. The model for IEmbedI1 should not cause us to model methods on I1 - if the user wants that then they can just model I1 in the first place. I will look into this as a follow-up.

@smowton
Copy link
Contributor

smowton commented Sep 24, 2024

Suggest change the PR title and description before merging, and add a change note, since it now actually fixes a bug.

@@ -0,0 +1,89 @@
package main

import (
Copy link
Contributor

@smowton smowton Sep 24, 2024

Choose a reason for hiding this comment

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

Recommend using the SPURIOUS attribute throughout (and if applicable MISSING, though I don't think there are any missing results) to identify what ought to be fixed

@owen-mc owen-mc changed the title Go: Add tests for model inheritance Go: Add tests for model inheritance and fix bug in promoted methods Sep 24, 2024
@owen-mc owen-mc removed the no-change-note-required This PR does not need a change note label Sep 24, 2024
@owen-mc
Copy link
Contributor Author

owen-mc commented Sep 26, 2024

DCA with the normal source suite didn't show any changes. I also ran it with the suite @smowton made recently of repos which exercise our models more (which is much larger at 129 repos). This showed alert losses in 7 repos. I looked into them. They almost all seem to relate to getACall being too broad (a known problem which we plan to fix). Most of them were logging queries. So I am happy to accept these alert losses.

Copy link
Contributor

@smowton smowton left a comment

Choose a reason for hiding this comment

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

I think the spurious annotations are correct, though it's hard to be 100% certain. Once the fixes start to roll in it'll become clear if we see anything other than the expected spurious -> absent transitions.

@owen-mc owen-mc merged commit fdff209 into github:main Sep 26, 2024
15 checks passed
@owen-mc owen-mc deleted the go/inheritance-tests branch September 26, 2024 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants