Skip to content

Go: sync ExternalFlow.qll #16558

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 29 commits into from
Jun 5, 2024
Merged

Go: sync ExternalFlow.qll #16558

merged 29 commits into from
Jun 5, 2024

Conversation

owen-mc
Copy link
Contributor

@owen-mc owen-mc commented May 22, 2024

Update go's copy of ExternalFlow.qll to bring it in line with java and csharp's.

  • Add support for neutral models.
  • Change the way that the receiver is referred to in models-as-data models. Previously it was Argument[-1]. Now it is Argument[receiver]. (Other languages use different terminology, such as Argument[this].)

@github-actions github-actions bot added the Go label May 22, 2024
@owen-mc owen-mc force-pushed the go/sync-external-flow branch 2 times, most recently from d9dfb93 to 9c2667b Compare May 23, 2024 10:24
@owen-mc owen-mc marked this pull request as ready for review May 23, 2024 13:26
@owen-mc owen-mc requested a review from a team as a code owner May 23, 2024 13:26
private predicate canonicalPackageHasASubpackage(string package, string subpkg) {
canonicalPackage(package) and
(subpkg = package or packageLink(package, subpkg))
}

/**
* Holds if MaD framework coverage of `package` is `n` api endpoints of the
* kind `(kind, part)`, and `pkgs` is the number of subpackages of `package`
* which have MaD framework coverage (including `package` itself).
* kind `(kind, part)`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this stop documenting pkgs, which still exists?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To match the java and C# versions of this file, so they're easier to keep in sync or make into a shared library. You're right that it's better though. Maybe I'll keep it and make a tiny PR to add it to the others.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done here: #16676

) and
not exists(Provenance provenance |
neutralElement(this, "summary", provenance) and
provenance.isManual()
Copy link
Contributor

Choose a reason for hiding this comment

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

isGenerated

Copy link
Contributor Author

@owen-mc owen-mc Jun 4, 2024

Choose a reason for hiding this comment

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

Actually it is correct. The idea is that generated summary models are overridden by manual neutral models. This allows us to fixed generated models in a way which will persist when they are regenerated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know if you think there should be a comment briefly explaining this (and feel free to give a suggestion).

@smowton
Copy link
Contributor

smowton commented Jun 3, 2024

Looks plausible, except as noted

@owen-mc owen-mc force-pushed the go/sync-external-flow branch from 97807e6 to cbbdd01 Compare June 4, 2024 10:52
@owen-mc
Copy link
Contributor Author

owen-mc commented Jun 4, 2024

I've addressed the review comments and rebased to check that this is still in a mergeable state, since it was quite old. There has been one change to the java copy of ExternalFlow.qll since I made the PR but it isn't relevant to go so I'll leave it for now.

@owen-mc owen-mc merged commit 44a56c4 into github:main Jun 5, 2024
@owen-mc owen-mc deleted the go/sync-external-flow branch June 5, 2024 10:31
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