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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Copy metadata associated with a node in the optimized result #6593

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open
56 changes: 56 additions & 0 deletions ast/annotations.go
Expand Up @@ -111,6 +111,11 @@ func (a *Annotations) EndLoc() *Location {
return a.comments[count-1].Location
}

// Comments returns the comments associated with this annotation.
func (a *Annotations) Comments() []*Comment {
return a.comments
}

// Compare returns an integer indicating if a is less than, equal to, or greater
// than other.
func (a *Annotations) Compare(other *Annotations) int {
Expand Down Expand Up @@ -181,6 +186,26 @@ func (a *Annotations) GetTargetPath() Ref {
}
}

func (a *Annotations) GetPackage() *Package {
switch n := a.node.(type) {
case *Package:
return n
case *Rule:
return n.Module.Package
default:
return nil
}
}

func (a *Annotations) GetRule() *Rule {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there situations where a == nil? attachRuleAnnotations() seems to rely on this fact. Could we simplify that function by simply iterating over all the module's annotations, call GetRule() on them, and attach the annotation to any non-nil return value?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, never mind. I just realized we need to attach document-scoped annotations to all rules with the same path, and n would only represent one of those.

I suppose we have a way to make sure that we don't emit the document-scope annotation block for each moved rule when rewriting 馃 . Or maybe that makes little difference, as they'll be identical anyways ..

switch n := a.node.(type) {
case *Rule:
return n
default:
return nil
}
}

func (a *Annotations) setJSONOptions(opts astJSON.Options) {
a.jsonOptions = opts
if a.Location != nil {
Expand Down Expand Up @@ -509,6 +534,37 @@ func (a *Annotations) toObject() (*Object, *Error) {
return &obj, nil
}

func attachRuleAnnotations(mod *Module) {
// make a copy of the annotations
cpy := make([]*Annotations, len(mod.Annotations))
for i, a := range mod.Annotations {
cpy[i] = a.Copy(a.node)
}

for _, rule := range mod.Rules {
var j int
var found bool
for i, a := range cpy {
if rule.Loc().Row > a.Location.Row {
if rule.Ref().Equal(a.GetTargetPath()) {
rule.Annotations = append(rule.Annotations, a)

if a.Scope == annotationScopeRule {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do I get this right?:

  • if there is a document-scoped annotation block, and it's declared above a rule-scope annotation block, it'll get removed from cpy.
  • if it's instead declared after all rule-scoped annotations, it'll be left in cpy?

This is probably not an actual issue, as we also match the rule ref and annotation target path, so any straggling document-scoped annotations won't be attached to any succeeding rules with unmatched ref/path.

Copy link
Contributor

Choose a reason for hiding this comment

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

@anderseknert, is there a regal rule for having multiple rule-scope annotation blocks for a rule? If not, might be a good candidate for a new linting rule, as this is a likely user mistake.

Copy link
Member Author

Choose a reason for hiding this comment

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

if there is a document-scoped annotation block, and it's declared above a rule-scope annotation block, it'll get removed from cpy.
if it's instead declared after all rule-scoped annotations, it'll be left in cpy?
This is probably not an actual issue, as we also match the rule ref and annotation target path, so any straggling document-scoped annotations won't be attached to any succeeding rules with unmatched ref/path.

My thinking here is: We attach a rule-scope annotation block that precedes the rule to that specific rule. That annotation will NOT apply to any other rule. So we remove it from cpy. We keep the document-scope annotations as they could apply to other rules as well. Does this make sense?

j = i
found = true
}
}
} else {
break
}
}

if found && j < len(cpy) {
cpy = append(cpy[:j], cpy[j+1:]...)
}
}
}

func attachAnnotationsNodes(mod *Module) Errors {
var errs Errors

Expand Down
1 change: 1 addition & 0 deletions ast/check_test.go
Expand Up @@ -2328,6 +2328,7 @@ p { input = "foo" }`}},
as, errors := BuildAnnotationSet(modules)
typeenv, checkErrors := newTypeChecker().WithSchemaSet(schemaSet).CheckTypes(oldTypeEnv, elems, as)
errors = append(errors, checkErrors...)

if len(errors) > 0 {
for _, e := range errors {
if tc.err == "" || !strings.Contains(e.Error(), tc.err) {
Expand Down
4 changes: 3 additions & 1 deletion ast/compile.go
Expand Up @@ -2191,11 +2191,13 @@ func (c *Compiler) parseMetadataBlocks() {

if len(mod.Annotations) == 0 {
var errs Errors
mod.Annotations, errs = parseAnnotations(mod.Comments)
mod.Annotations, errs = ParseAnnotations(mod.Comments)
errs = append(errs, attachAnnotationsNodes(mod)...)
ashutosh-narkar marked this conversation as resolved.
Show resolved Hide resolved
for _, err := range errs {
c.err(err)
}

attachRuleAnnotations(mod)
}
}
}
Expand Down
16 changes: 16 additions & 0 deletions ast/marshal_test.go
Expand Up @@ -444,6 +444,22 @@ func TestRule_MarshalJSON(t *testing.T) {
}(),
ExpectedJSON: `{"body":[{"index":0,"terms":{"type":"boolean","value":true}}],"head":{"name":"allow","value":{"type":"boolean","value":true},"ref":[{"type":"var","value":"allow"}]},"location":{"file":"example.rego","row":6,"col":2}}`,
},
"annotations included": {
Rule: func() *Rule {
r := rule.Copy()
r.Annotations = []*Annotations{{
Scope: "rule",
Title: "My rule",
Entrypoint: true,
Organizations: []string{"org1"},
Description: "My desc",
Custom: map[string]interface{}{
"foo": "bar",
}}}
return r
}(),
ExpectedJSON: `{"annotations":[{"custom":{"foo":"bar"},"description":"My desc","entrypoint":true,"organizations":["org1"],"scope":"rule","title":"My rule"}],"body":[{"index":0,"terms":{"type":"boolean","value":true}}],"head":{"name":"allow","value":{"type":"boolean","value":true},"ref":[{"type":"var","value":"allow"}]}}`,
},
}

for name, data := range testCases {
Expand Down
4 changes: 2 additions & 2 deletions ast/parser.go
Expand Up @@ -433,7 +433,7 @@ func (p *Parser) Parse() ([]Statement, []*Comment, Errors) {

func (p *Parser) parseAnnotations(stmts []Statement) []Statement {

annotStmts, errs := parseAnnotations(p.s.comments)
annotStmts, errs := ParseAnnotations(p.s.comments)
for _, err := range errs {
p.error(err.Location, err.Message)
}
Expand All @@ -445,7 +445,7 @@ func (p *Parser) parseAnnotations(stmts []Statement) []Statement {
return stmts
}

func parseAnnotations(comments []*Comment) ([]*Annotations, Errors) {
func ParseAnnotations(comments []*Comment) ([]*Annotations, Errors) {

var hint = []byte("METADATA")
var curr *metadataParser
Expand Down
2 changes: 2 additions & 0 deletions ast/parser_ext.go
Expand Up @@ -713,6 +713,8 @@ func parseModule(filename string, stmts []Statement, comments []*Comment, regoCo
return nil, errs
}

attachRuleAnnotations(mod)

return mod, nil
}

Expand Down