Restrictions do not apply to class literals, `instanceof`, casts, or method references

[dwnusbaum]

I am not sure if this is intended, but restrictions do not apply to class literals (XYZ.class) (EDIT: and a few other cases, see https://github.com/jenkinsci/lib-access-modifier/issues/166#issuecomment-1686950515). For example, access-modifier-checker does not fail given this in one module:

@Restricted(NoExternalUse.class)
class Foo { }

And this in another module:

class Bar {
  Bar() {
    Foo.class.getName();
  }
}

I think it is straightforward to fix this if we do consider it a bug by expanding Checker.RestrictedMethodVisitor to override visitLdcInsn, although I would not be surprised if plugins are inadvertently relying on the current behavior, making this a source-incompatible change.

[daniel-beck]

I would not be surprised if plugins are inadvertently relying on the current behavior, making this a source-incompatible change

Given how we use it in core, e.g. freely renaming/refactoring/removing things, this seems like a net positive change, +1 from me. (Compare restricting generated Messages.)

[jglick]

I would not be surprised if plugins are inadvertently relying on the current behavior

I am pretty sure I have relied on the current behavior.

[dwnusbaum]

For what it's worth, looking a little deeper, restrictions also don't apply when casting (Foo) foo, with instanceof (foo instanceof Foo), or with method references (Runnable r = Foo::restrictedMethod; r.run()).

foo.class, foo instanceof Foo, and (Foo) foo seem more or less the same, so I think we should probably either block all of them or none of them.

Method references seem distinct and for those I don't really see any reason to allow them. It is a bit awkward to work with them though using ASM.

I am pretty sure I have relied on the current behavior.

Are you just noting that as far as concern about potential incompatibilities, or do you mean that you think we should preserve the current behavior?

[basil]

I don't think we should be making breaking changes without proactively identifying and adapting consumers.

[jglick]

I am not too concerned since this is only a build-time error and you can always suppress this tool on a per-class level.

[basil]

To be absolutely clear, I intend to block this change unless consumers are proactively identified and adapted.