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

[apex] Add a rule to prevent use of non-existent annotations #836

Merged
merged 13 commits into from Jun 24, 2018

Conversation

Projects
None yet
4 participants
@anand13s
Contributor

anand13s commented Jan 10, 2018

This rule seeks to warn against using non-existent annotations in apex source code. A 'non existent' annotation is one not defined in the apex language documentation annotations section.

This is done to prevent apex source code from being broken by newer Apex annotations in the Salesforce language that possess the same signature as a non existent one.

@anand13s anand13s closed this Jan 10, 2018

@anand13s anand13s reopened this Jan 10, 2018

Merge remote-tracking branch 'Up2Go/master'
# Conflicts:
#	pmd-apex/src/main/resources/rulesets/apex/ruleset.xml
@jsotuyod

@anand13s thanks for the PR! The rule looks like a nice addition, and the way to get valid annotations by reflection from Jorje is neat (are we certain it includes all accepted annotations?).

Please, rebase against master and check the issues I here raise. Let me know if you need any assistance.

import net.sourceforge.pmd.renderers.CodeClimateRule;
public abstract class AbstractApexRule extends AbstractRule
implements ApexParserVisitor, ImmutableLanguage, CodeClimateRule {
public static final IntegerProperty CODACY_MINUTES_TO_FIX = new IntegerProperty("codacy_minutes_to_fix",

This comment has been minimized.

@jsotuyod

jsotuyod Jan 10, 2018

Member

the codacy properties are unrelated to the object of this PR, please revert this changeset

@@ -0,0 +1,88 @@
package net.sourceforge.pmd.lang.apex.rule.style;

This comment has been minimized.

@jsotuyod

jsotuyod Jan 10, 2018

Member

the rule seems like a better fit for the errorprone category (code that is likely to break) rather than a code style preference. Consider moving it there.

@@ -1,128 +1,180 @@
<?xml version="1.0" encoding="UTF-8"?>
<ruleset xmlns="http://pmd.sourceforge.net/ruleset/2.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" name="Default ruleset used by the CodeClimate Engine for Salesforce.com Apex" xsi:schemaLocation="http://pmd.sourceforge.net/ruleset/2.0.0 http://pmd.sourceforge.net/ruleset_2_0_0.xsd">

This comment has been minimized.

@jsotuyod

jsotuyod Jan 10, 2018

Member

changes to this file are unrelated to the object of this PR, please revert

@@ -132,4 +132,24 @@ Many interfaces (e.g. Batch) required global modifiers in the past but don't req
</example>
</rule>
<rule name="AvoidNonExistentAnnotations"

This comment has been minimized.

@jsotuyod

jsotuyod Jan 10, 2018

Member

rulesets are deprecated. New rules should be added to categories. In particular, this looks like a fit for the errorprone category

@@ -132,4 +132,24 @@ Many interfaces (e.g. Batch) required global modifiers in the past but don't req
</example>
</rule>
<rule name="AvoidNonExistentAnnotations"
since="5.5.5"

This comment has been minimized.

@jsotuyod

jsotuyod Jan 10, 2018

Member

the next release to include new rules will be 6.1.0

@@ -0,0 +1,55 @@
<?xml version="1.0" encoding="UTF-8"?>

This comment has been minimized.

@jsotuyod

jsotuyod Jan 10, 2018

Member

consider adding test scenarios where valid annotations are not flagged

end inline_praqma_error;
/
create or replace package inline_praqma_error is

This comment has been minimized.

@jsotuyod

jsotuyod Jan 10, 2018

Member

this changeset is unrelated to the object of the PR, please revert

loFPOGE_OBJ:=SELECT A FROM persons p WHERE IS OF TYPE (employee_t);
loFPOGE_OBJ:=SELECT A FROM persons p WHERE IS NOT OF TYPE (ONLY employee_t, other_t);
PROCEDURE IsOfType (

This comment has been minimized.

@jsotuyod

jsotuyod Jan 10, 2018

Member

this changeset is unrelated to the object of the PR, please revert

@jsotuyod jsotuyod added the a:new-rule label Jan 10, 2018

@jsotuyod jsotuyod changed the title from [Apex] Add a rule to prevent use of non-existent annotations to [apex] Add a rule to prevent use of non-existent annotations Jan 10, 2018

@capeterson

This comment has been minimized.

capeterson commented Jan 10, 2018

@jsotuyod I'm the product manager for Apex at Salesforce (Anand is on the dev team), and wanted to answer

are we certain it includes all accepted annotations?

This will be the case for the foreseeable future. I can't make an official guarantee as this isn't a formally supported API, but we have no current plans to make changes that would stop this from being true.

<description>
Apex supported non existent annotations for legacy reasons.
In the future, use of such non-existent annotations could result in broken apex code that will not copile.
This will prevent users of garbage annotations from being able to use legitimate annotations added to apex in the future.

This comment has been minimized.

@jsotuyod

jsotuyod Jan 12, 2018

Member

consider adding the link to the list of valid annotations here for user reference

@jsotuyod

This comment has been minimized.

Member

jsotuyod commented Jan 12, 2018

@capeterson thanks for the input! I'm really glad to know that.

I'm looking forward to strengthen the collaboration between PMD and Salesforce. Feel free to contact me to better align objectives / priorities and improve synergies.

@rsoesemann

This comment has been minimized.

Contributor

rsoesemann commented Jan 12, 2018

@jsotuyod and @capeterson and please always feel free to pull me in as a middle man in case of timing issues. I still feel very responsible ;-)

@jsotuyod jsotuyod added the is:WIP label Jan 14, 2018

@rsoesemann

This comment has been minimized.

Contributor

rsoesemann commented Jan 28, 2018

@jsotuyod can I help to solve this conflict? Is there even still a conflict?

@jsotuyod

This comment has been minimized.

Member

jsotuyod commented Jan 28, 2018

@rsoesemann there are 2 merge-conflicts and still all my unresolved comments. I don't think you can solve this unless you take the branch into your PMD fork, and submit a new PR yourself.

PMD maintainers can do something similar on local copies before merging.

@rsoesemann

This comment has been minimized.

Contributor

rsoesemann commented Jan 29, 2018

@anand13s any reason for not responding to the comments and merge conflicts?

@capeterson

This comment has been minimized.

capeterson commented Jan 29, 2018

@rsoesemann he's been busy with his day job and all the work I heaped on him for our Summer release. We know we need to fix these before this will be accepted.

@jsotuyod jsotuyod added this to the 6.5.0 milestone Jun 24, 2018

@jsotuyod jsotuyod self-assigned this Jun 24, 2018

@jsotuyod jsotuyod removed the is:WIP label Jun 24, 2018

@jsotuyod jsotuyod merged commit 3f71e73 into pmd:master Jun 24, 2018

jsotuyod added a commit that referenced this pull request Jun 24, 2018

@jsotuyod

This comment has been minimized.

Member

jsotuyod commented Jun 24, 2018

I personally fixed the remaining issues and merged it. This will be part of PMD 6.5.0.

@rsoesemann

This comment has been minimized.

Contributor

rsoesemann commented Jun 26, 2018

Thanks everybody involved especially @jsotuyod for getting this into production.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment