-
Notifications
You must be signed in to change notification settings - Fork 19
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[JENKINS-53675] Add the ability to disable checks for certain classes only #16
Conversation
This allows you to disable @restricted class/methods at the point of use
/* | ||
* The MIT License | ||
* | ||
* Copyright (c) 2010, Kohsuke Kawaguchi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you probably want to change this :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks ok apart from the javadoc which looks wrong to me.
import static java.lang.annotation.RetentionPolicy.RUNTIME; | ||
|
||
/** | ||
* Indicates that a particular element is really deprecated and that the access to it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks wrong
@Target({ElementType.METHOD, ElementType.CONSTRUCTOR, ElementType.TYPE}) | ||
public @interface DisableRestriction { | ||
/** | ||
* Kind of access that are restricted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as does this.
@@ -228,122 +236,7 @@ public void checkClass(File clazz) throws IOException { | |||
FileInputStream in = new FileInputStream(clazz); | |||
try { | |||
ClassReader cr = new ClassReader(in); | |||
cr.accept(new ClassVisitor(Opcodes.ASM5) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
easier to review the pre-refactored diff
@jtnord - fixed the javadoc/headers and changed a bit because it wasn't quite working |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
JIRA appears unresponsive so I can't see the upstream discussion, so my comment is without the context. The central design idea of access modifier annotation like Also, when I think about other programming languages that have more complicated access modifiers, like So from that perspective, this mechanism of allowing arbitrary caller to ignore that intent seems to defeat the purpose, and I don't see any reference in code/documentation that explains how/why this should be used, so that makes me feel uneasy. With all that said, I'm coming in late, and like I said I don't have the context of JENKINS-53675 so I'm blind to the problem that led to this solution. And because of that, I can't offer more constructive suggestion, and I hate not being able to do that. This feedback questions the approach itself, and it's too late for that. So if others with more context see the logic in this, don't let my -1 stand in the way. |
As a plugin maintainer, while I've annotated my code with JENKINS-53675 unfortunately focus on the technical requirement, not the use-case behind this. |
Whilst I agree with the comments above, we have hit the issue where we need to use some In our specific case, we want to:
When using an At the moment, we are just disabling the |
bypassing |
Sure, here it is: jenkinsci/jenkins#3617 (with objections too...) |
The important thing here is that we do not want to disable the entire annotation checker. Sure we could have no violations by changing the code to purely use reflection to call the code, but that gives us even less comfort, or we could disable the checker entirely. Given a project using this may be consuming multiple libraries we may only want to accept the risk of using a
Using reflection and loosing compile time guarantees is a really bad thing to do, and one that may not be possible in future JVMs. This is not always possible. for example an upstream has rejected our attempts to remove the |
I get your point. I would suggest to rely on |
SuppressRestrictedWarnings mirrors java.lang.SuppressWarnings naming Moved to a separate package makes it less-available to users and puts more barriers in for use. This makes the user stop-and-think about what they are doing, rather than just turning the warning off.
https://issues.jenkins-ci.org/browse/JENKINS-53675
Add an
@DisableRestriction
annotation that can be used at either the class or method level to disable@Restricted
checks on a per-class basis.Example use:
For more use, see
it/disable-restrictions
integration test@jenkinsci/code-reviewers @amuniz @jtnord