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

ashutosh-narkar · 2024-02-16T00:27:58Z

🚧 Fixes: #6529

ashutosh-narkar · 2024-02-16T00:32:05Z

ast/check_test.go

+		//# scope: document
+		//# schemas:
+		//# - input.foo: schema.string
+		//p { input.foo = "str" }`}},


I commented out this test. Since we now build the annotation set during module parsing, this test fails during the parse phase. I guess the question is should a module with incorrect metadata should pass module parsing when say ProcessAnnotation is true?

I've uncommented this test case. I've updated how the annotations are attached to the rule w/o building the annotation set during module parsing.

ast/parser_ext.go

ashutosh-narkar · 2024-02-16T00:35:09Z

cmd/build_test.go

+
+# METADATA
+# entrypoint: true


Should this have been omitted in the optimized module since the optimized package was an entrypoint?

Hm, getting an extra entrypoint at test.p.foo isn't ideal, but I don't think it's a deal-breaker. What's likely worse is that we've dropped the probably expected test.p entrypoint.

I don't know if it's easily achieved, but maybe we could make it so that we don't shift path components from the rule to the package if there are annotations on the rule. I.e. we should produce the support module:

package test # METADATA # entrypoint: true p.foo contains __local1__1 if { __local1__1 = input.v }

I've updated the code so that we no longer add an extra entrypoint. We currently drop the existing entrypoint. I'll see if we can change that.

We are maintaining current behavior here in terms of not annotating a shifted optimized entrypoint. Can we handle that as part of a separate change? We can probably add a note in the docs for now. WDYT @johanfylling?

Sure.

Would be good if we could inform the user about this, somehow. We don't have much in ways of logging when building a bundle, right?
Maybe we could leave a comment in the original source file if we've dropped an entrypoint. E.g.:

# NOTE: Entrypoint foo/bar/baz has been dropped due to build optimization.

Would be good if we could inform the user about this, somehow. We don't have much in ways of logging when building a bundle, right?

We already do that if you run opa build with the --debug flag. So for the above policy we log a message like

optimizer: entrypoint: data.test.p: discard due to self-reference

WDYT?

ashutosh-narkar · 2024-02-16T00:36:22Z

compile/compile.go

@@ -908,6 +967,8 @@ func (o *optimizer) Do(ctx context.Context) error {
 	// because otherwise the optimization outputs (e.g., support rules) would have to
 	// merged somehow. Instead of dealing with that, just run the optimizations in the
 	// order the user supplied the entrypoints in.
+
+	var flattenedAnnotations []*ast.AnnotationsRef


We should probably check the optimization level maybe and only do this for O=1?

Should be ok to do this w/o any explicit check.

ashutosh-narkar · 2024-02-16T00:37:56Z

format/format.go

+
+		if len(existingAnnotations) == 0 {
+			for _, an := range rule.Annotations {
+				w.blankLine()


This will sometimes add an additional blank line. From this part in the code it's not possible to figure out what the previous line is unless we read the buffer.

topdown/eval.go

johanfylling

I think the chosen approach is sound 👍 .
I had expected attaching annotations to rules (and packages) would simplify annotation/comment processing in places, but maybe that's not fully possible without breaking changes. E.g. we could maybe get rid of the module's annotations, but that would be a breaking change.

johanfylling · 2024-02-19T10:20:35Z

ast/annotations.go

+					rule.Annotations = append(rule.Annotations, a)
+					j = i
+					found = true
+					break


What if a rule has multiple sets of annotations, e.g. rule- and document-scope? Aren't all but the first dropped, to possibly be attached to the next rule?

Perhaps we should add a test for this function, asserting a couple of edge cases.

Updated this logic and added test cases.

ast/annotations.go

ast/compile.go

johanfylling · 2024-02-19T12:10:19Z

ast/policy.go

+		Body        Body           `json:"body"`
+		Else        *Rule          `json:"else,omitempty"`
+		Location    *Location      `json:"location,omitempty"`
+		Annotations []*Annotations `json:"-"`


Would exposing the annotations in the AST-json (partially) solve #6280?
@anderseknert, any thoughts?

Yes, seems like a good opportunity to kill two birds with one stone 👍 We could always gate it behind one of the many JSON options we have available now.

We're exposing the annotations in the AST-json.

johanfylling · 2024-02-19T12:21:58Z

cmd/build_test.go

+
+# METADATA
+# entrypoint: true


Hm, getting an extra entrypoint at test.p.foo isn't ideal, but I don't think it's a deal-breaker. What's likely worse is that we've dropped the probably expected test.p entrypoint.

cmd/build_test.go

johanfylling · 2024-02-19T12:45:37Z

compile/compile.go

+			for _, rule := range mf.Parsed.Rules {
+				if p.Equal(rule.Ref()) {
+					annotations = append(annotations, annotation)
+					found = true


A rule might have been dropped from the bundle through optimization here?

Not sure I understand. We run this on the optimized bundle. Can you please elaborate?

johanfylling · 2024-02-19T13:04:04Z

format/format.go

 	comments = w.insertComments(comments, pkg.Location)

 	w.startLine()
+
+	if len(parsedAnnotations) == 0 {


When parsing annotations, corresponding comments are dropped?
There is no risk of (len(annotations) > 0 && len(parsedAnnotations) > 0) == true?

When parsing annotations, corresponding comments are dropped?

While parsing the annotations the corresponding comments are NOT dropped. In cases where the policies are optimized, we now attach the annotations to the optimized rule and module AST. But in this case there are NO comments on the optimized module (ie. the comments from the original module are not carried over to the optimized module but NOW annotations are). So in my testing I haven't come across this condition (len(annotations) > 0 && len(parsedAnnotations) > 0) == true.

Can you imagine a scenario where this is likely?

No, since the support module should never have any comments, this should be fine, I think 👍.

johanfylling · 2024-02-19T13:14:34Z

format/format.go

+				w.writeComments(an.Comments())
+			}
+		}
+
 		comments = w.insertComments(comments, rule.Location)


I believe annotation parsing will break if there isn't an empty line separating annotations from trailing comments. w.insertComments() ensures there is a blank line between the comments and any annotations written above?

Good catch. I guess we can check for trailing comments and add a blank line after writing out the annotations. Updated the code.

topdown/eval.go

johanfylling

Just a few thoughts/comments.

johanfylling · 2024-03-06T08:46:02Z

ast/annotations.go

+		var found bool
+		for i, a := range cpy {
+			if rule.Loc().Row > a.Location.Row {
+				if rule.Ref().Equal(a.GetTargetPath()) && (a.Scope == annotationScopeRule || a.Scope == annotationScopeDocument) {


Since we require the annotation's node ref to match the rule's ref, is there actually a need in asserting the annotation's scope, as we already know it belongs to the rule? If we skipped this check, we'd even be a bit more future proof here if we ever were to add new scopes (albeit unlikely).

You're right we probably don't need that check. Updated code.

johanfylling · 2024-03-06T08:54:55Z

ast/annotations.go

+				if rule.Ref().Equal(a.GetTargetPath()) && (a.Scope == annotationScopeRule || a.Scope == annotationScopeDocument) {
+					rule.Annotations = append(rule.Annotations, a)
+
+					if a.Scope == annotationScopeRule {


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.

@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.

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?

johanfylling · 2024-03-06T09:02:32Z

ast/annotations.go

+	}
+}
+
+func (a *Annotations) GetRule() *Rule {


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?

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 ..

johanfylling · 2024-03-06T09:34:36Z

ast/parser_test.go

+	a2 := []*Annotations{
+		{
+			Description: "doc",
+			Scope:       "document",


Ah, ok, this is why we don't remove document-scope annotations from cpy in attachRuleAnnotations() 👍 .

johanfylling · 2024-03-06T12:26:20Z

ast/parser_test.go

+import rego.v1
+
+# METADATA
+# scope: document


To be completionists, should we add tests for when the doc-scope annotations are preceded by rule-scope annotations for the same rule?

Added a test case for this.

johanfylling · 2024-03-06T13:09:16Z

compile/compile.go

+			}
+		}
+
+		if module := o.getSupportForEntrypoint(pq.Queries, e, resultsym, flattenedAnnotations); module != nil {


If we did the call to getSupportForEntrypoint() before the above, would we still need to attach annotations inside that function? Or would reordering screw something up?

Annotations wouldn't be attached to the new support rule, though .. so perhaps that's not an option 🤔.

Annotations wouldn't be attached to the new support rule, though

Yes.

johanfylling · 2024-03-06T13:15:00Z

compile/compile.go

+		rule := &ast.Rule{ // TODO(sr): use RefHead instead?
+			Head:        ast.NewHead(name, nil, resultsym),
+			Body:        query,
+			Annotations: ruleAnnotations,


For the sake of homogeneity, should we attach the support rule to the annotations' node pointer here?

johanfylling · 2024-03-06T13:33:07Z

compile/compile_test.go

+				"optimized/test.rego": `
+					package test
+
+					foo = __result__ { __result__ = {"bar": {"p": true}} }`,


Ideally, we wouldn't do this kind of reformatting if the rule has annotations, right? We should perhaps create a ticket for fixing this.

johanfylling · 2024-03-06T13:43:53Z

compile/compile_test.go

+# description: r
+r = true { t with q as {3} }`,
+			},
+		},


For completeness sake, we should add tests for the document and subpackages scopes too.

I wonder if the subpackages scope will cause problems if it's emitted for each new support module, as that might cause a redeclaration error .. 🤔.
That might however only be the case if the annotations differ. I forget.

johanfylling · 2024-03-06T13:54:34Z

format/format.go

 	comments = w.insertComments(comments, pkg.Location)

 	w.startLine()
+
+	if len(parsedAnnotations) == 0 {


No, since the support module should never have any comments, this should be fine, I think 👍.

Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>

stale · 2024-04-10T02:18:44Z

This pull request has been automatically marked as stale because it has not had any activity in the last 30 days.

ashutosh-narkar commented Feb 16, 2024

View reviewed changes

ast/parser_ext.go Outdated Show resolved Hide resolved

ashutosh-narkar commented Feb 16, 2024

View reviewed changes

topdown/eval.go Outdated Show resolved Hide resolved

ashutosh-narkar requested a review from johanfylling February 16, 2024 00:38

johanfylling reviewed Feb 19, 2024

View reviewed changes

ashutosh-narkar force-pushed the move-metadata-optimize branch 2 times, most recently from 4badf68 to 9f701a7 Compare February 22, 2024 23:28

ashutosh-narkar force-pushed the move-metadata-optimize branch from ba2448a to 40b4ab7 Compare February 27, 2024 19:42

ashutosh-narkar changed the title ~~WIP: Copy metadata associated with a node in the optimized result~~ Copy metadata associated with a node in the optimized result Feb 27, 2024

ashutosh-narkar force-pushed the move-metadata-optimize branch from 40b4ab7 to 9bd3afa Compare February 28, 2024 19:50

johanfylling reviewed Mar 6, 2024

View reviewed changes

ashutosh-narkar force-pushed the move-metadata-optimize branch from 9bd3afa to 4d58403 Compare March 7, 2024 22:25

ashutosh-narkar added 13 commits March 7, 2024 14:45

WIP: Copy metadata associated with a node in the optimized result

0c32d66

Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>

Attach rule annotations w/o building annotation set

deadc77

Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>

deep copy annotations

3dd08a8

Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>

attach rule annos while parsing metadata

0b197b0

Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>

Add and update tests

6107e09

Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>

fix lint checks

8594f86

Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>

update code flow for pruning annotations

5cae5d0

Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>

add blank line after writing annotations

525f449

Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>

Add a conditional for blank line insertion

64ef639

Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>

Add a conditional for blank line insertion-2

cbeea74

Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>

More testing for ast, json

4ccd384

Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>

add test case for format package

bd10ba5

Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>

address pr comments-1

31fe465

Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>

ashutosh-narkar force-pushed the move-metadata-optimize branch from 4d58403 to 31fe465 Compare March 8, 2024 00:46

stale bot added the inactive label Apr 10, 2024

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

Are you sure you want to change the base?

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

Conversation

ashutosh-narkar commented Feb 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johanfylling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johanfylling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Apr 10, 2024