Go: Allow grouping import paths for models-as-data #16941
Conversation
Click to show differences in coveragegoGenerated file changes for go
- `beego <https://beego.me/>`_,"``github.com/astaxie/beego*``, ``github.com/beego/beego*``",63,49,
+ `beego <https://beego.me/>`_,"``github.com/astaxie/beego*``, ``github.com/beego/beego*``",63,63,
- Totals,,83,884,24
+ Totals,,83,898,24
- github.com/beego/beego/context,,15,,,,15,,
+ github.com/beego/beego/context,,15,1,,,15,1,
+ github.com/beego/beego/utils,,,13,,,,13, |
|
All test failures are now changes in provenance information, which I will fix at the last minute, so this is ready to review. |
| FlowExtensions::summaryModel(p, type, subtypes, name, signature, ext, input, output, kind, | ||
| provenance, madId) | ||
| | | ||
| not exists(string s | p = groupPrefix() + s) and package = p |
There was a problem hiding this comment.
Just not p.matches(groupPrefix() + "%")?
There was a problem hiding this comment.
It's very hard to know what's faster. I'm pretty sure that what I currently have is dealt with well by the optimizer. But I remember now that QL strings have a prefix() method, which surely must be performant, so I'll use that.
There was a problem hiding this comment.
The packageOrGroup.prefix(groupPrefix().length()) = groupPrefix() should actually be expected to be worse than both of those other options, since it forces the evaluator to manifest all those packageOrGroup-prefixes in the string pool. The matches-version is fine, but I expect exists(string s | p = groupPrefix() + s) to be the best version. We really need a builtin string.startsWith predicate to make it such that the nicest version is also the best version for performance.
| string groupPrefix() { result = "group:" } | ||
|
|
||
| /** Gets a group that `package` is in, according to `packageGrouping`. */ | ||
| private string getGroup(string package) { |
There was a problem hiding this comment.
Suggest call this getPackage(string packageOrGroup), and push the "if is group then resolve, else result = input" logic in here (with a bindingset, so it gets inlined at use sites).
| /** | ||
| * Holds if a source model exists for the given parameters. | ||
| * | ||
| * Note that we consider all packages in the same group. |
There was a problem hiding this comment.
| * Note that we consider all packages in the same group. | |
| * Note that `group:` references are expanded into one or more actual packages by this predicate. |
(here and elsewhere)
This could be made public in future. But I expect that we will want to use this logic for QL models as well then we will want to move it into a different file, which will be much easier if it's all private at the moment.
owen-mc
force-pushed
the
go/mad-package-alias
branch
from
f1d818b to
2c7fbda
Compare
This PR removes the need for duplicate models for different package paths. It's quite common in Go that we want our models to apply to multiple package import paths, e.g. Beego used to be at github.com/astaxie/beego, then it moved to github.com/beego/beego, and then it released v2 and moved some folders around, so that package is now at github.com/beego/beego/v2/server/web (which we model as github.com/beego/beego/server/web so that we get all future versions as well). Currently we have to have three copies of all models, e.g.
This duplication makes it hard to be sure that we have all the models for all the import paths. It would be much better to introduce a way of specifying which package paths to use separately. With this PR, that becomes