Strict mode: fail on unused imports [PR]

[damienjburks]

Fixes #4354

[srenatus]

😃 Thanks for picking this up! Welcome to compiler land 🎉

This is definitely going in a good direction. I've added a few comments inline, please let me know if you'd like me to add more detail to them.

[srenatus]

Oh that's an interesting approach. Took me a moment to understand 😃

I think we'd be better off using one of the Walk* methods here (ast.WalkTerms or ast.WalkExprs or whatever fits); because right now, this check is prone to false positives:

package p

import data.foo.bar as bar

r {
  input.bar == 2
}

[damienjburks]

I'm making those requested changes, but I have a question for you. When you say it's a false positive, for this case, that means that we should be checking for if it is an exact match or the postfix of the variable is equivalent to the input?

For example,

input.bar == 2 -> OK
hello.bar == 2 -> OK
hello.wut == 2 -> import data.foo.bar as bar unused.

[srenatus]

Sorry if false positive was a the wrong word -- I mean that when the compiler runs this stage, it should be clear if we're using the import, unambiguously. So, neither input.bar nor hello.bar should not make the compiler think that import data.foo.bar as bar (or a plain import data.foo.bar) was used.

I often find myself adding log.Printf("%s: %v", s.name, c.Modules) to this function to figure out what the compiler does in rewriting. With a module like the example, we'll find

2022/02/28 08:25:43 CheckDuplicateImports: map[p.rego:package p

import data.foo.bar as bar

r = true { equal(input.bar, 2) }]
2022/02/28 08:25:43 CheckKeywordOverrides: map[p.rego:package p

import data.foo.bar as bar

r = true { equal(input.bar, 2) }]
2022/02/28 08:25:43 ResolveRefs: map[p.rego:package p

r = true { equal(input.bar, 2) }]

So ResolveRefs has removed the import, and didn't otherwise change the module.

With a positive example,

package p

import data.foo.bar

r {
  input.bar == bar
}

we get this:

2022/02/28 08:27:29 CheckDuplicateImports: map[q.rego:package p

import data.foo.bar

r = true { equal(input.bar, bar) }]
2022/02/28 08:27:29 CheckKeywordOverrides: map[q.rego:package p

import data.foo.bar

r = true { equal(input.bar, bar) }]
2022/02/28 08:27:29 ResolveRefs: map[q.rego:package p

r = true { equal(input.bar, data.foo.bar) }]

So we should be able to find unused imports by

• for all imports
  • take the ref of the import (qualified name won't matter because it's been rewritten already)
  • check if the ref is used in the body
  • fail if it isn't

This is merely a sketch, and there may be better ways to approach finding unused imports here.

Concretely, I think what I've outlined would fail with this example:

package p
import data.foo # unused
r { data.foo == 10 }

because at the stage we'd check for used imports, the import would already have been removed, but it didn't actually resolve any ref.

This makes me think that perhaps finding unused imports should be a stage that closely mirrors what ResolveRefs does, instead of running after ResolveRefs. What do you think?

(Btw these would all make decent test cases when you get to adding a few test cases. Personally, I prefer to start with them, because it gives me some assurance when expanding the functionality that previous wins aren't lost again 😅)

[damienjburks]

For sure. I understand your concept. I uploaded some new code that is passing all of the test cases I could come up with. Please check it out and let me know what you think.

[anderseknert]

I'll leave the review in @srenatus capable hands, but just one thing I noticed - could you change "Addresses" to "Fixes" in the commit message? That'll make the script that builds the release notes pick it up automatically later :)

[srenatus]

Thanks for working on this! This is a good step into the right direction, please see my inline comments. 😃

[srenatus]

If you get around to it, adding a row to the strict-mode docs would be great, too: https://github.com/open-policy-agent/opa/blob/main/docs/content/strict.md#strict-mode-constraints-and-checks

[damienjburks]

@srenatus I've added those changes to the documentation. Please check when you have time. Everything seems to be working now.

[anderseknert]

@damienjburks awesome work on this!

Could you please rebase from the latest main, and fix the merge conflict that seems to be there?

[damienjburks]

@anderseknert rebased and fixed issues with broken test cases. Please review and let me know if there are any more changes I should make.

[anderseknert]

Thanks Damien!

It still looks like there's some changes in your PR that shouldn't be there (i.e. the change to philosophy.md). From your checked out fork, something like this should do the trick:

git checkout main
git remote add upstream https://github.com/open-policy-agent/opa.git
git pull upstream main
git checkout issue-4354
git rebase main
git push --force

[damienjburks]

@anderseknert just made those changes and pushed the latest code. Thanks for the guidance.

[srenatus]

I'll merge this, I've got some touch-ups that I'll open a PR for in a bit. Thanks again for picking this up and contributing @damienjburks 👏 🎉