Skip to content
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

[core] Select XPath version in violationSuppressXPath #1676

Open
Vampire opened this issue Feb 19, 2019 · 15 comments
Open

[core] Select XPath version in violationSuppressXPath #1676

Vampire opened this issue Feb 19, 2019 · 15 comments
Labels
an:enhancement An improvement on existing features / rules in:ruleset-xml About the ruleset schema/parser in:xpath Relating to xpath support at large, eg Jaxen / Saxon, custom functions, attribute resolution
Milestone

Comments

@Vampire
Copy link
Contributor

Vampire commented Feb 19, 2019

As the rule designer has a toggle for XPath 1.0 vs. 2.0, I assume XPath 2.0 should be supported?
Because if I use some @BeginColumn = (17, 60) in a violationSuppressXPath value, I get a syntax error.

java.lang.RuntimeException: XPath expression
                    .[(ancestor::ClassOrInterfaceDeclaration/@Image = 'DiscordWebhooksDiscordManager')
                        and (@BeginLine = 98)
                        and (@BeginColumn = (17, 60))]"
                 failed: Expected: )
        at net.sourceforge.pmd.lang.ast.AbstractNode.hasDescendantMatchingXPath(AbstractNode.java:432)
        at net.sourceforge.pmd.lang.rule.ParametricRuleViolation.setSuppression(ParametricRuleViolation.java:71)
        at net.sourceforge.pmd.lang.rule.ParametricRuleViolation.<init>(ParametricRuleViolation.java:54)
        at net.sourceforge.pmd.lang.java.rule.JavaRuleViolation.<init>(JavaRuleViolation.java:50)
        at net.sourceforge.pmd.lang.java.rule.JavaRuleViolationFactory.createRuleViolation(JavaRuleViolationFactory.java:24)
        at net.sourceforge.pmd.lang.rule.AbstractRuleViolationFactory.addViolation(AbstractRuleViolationFactory.java:37)
        at net.sourceforge.pmd.lang.rule.AbstractRule.addViolation(AbstractRule.java:359)
        at net.sourceforge.pmd.lang.rule.XPathRule.evaluate(XPathRule.java:116)
        at net.sourceforge.pmd.lang.java.rule.JavaRuleChainVisitor.visit(JavaRuleChainVisitor.java:42)
        at net.sourceforge.pmd.lang.rule.AbstractRuleChainVisitor.visitAll(AbstractRuleChainVisitor.java:96)
        at net.sourceforge.pmd.RuleChain.apply(RuleChain.java:67)
        at net.sourceforge.pmd.RuleSets.apply(RuleSets.java:140)
        at net.sourceforge.pmd.SourceCodeProcessor.processSource(SourceCodeProcessor.java:184)
        at net.sourceforge.pmd.SourceCodeProcessor.processSourceCode(SourceCodeProcessor.java:96)
        at net.sourceforge.pmd.SourceCodeProcessor.processSourceCode(SourceCodeProcessor.java:51)
        at net.sourceforge.pmd.processor.PmdRunnable.call(PmdRunnable.java:78)
        at net.sourceforge.pmd.processor.MonoThreadProcessor.runAnalysis(MonoThreadProcessor.java:29)
        at net.sourceforge.pmd.processor.AbstractPMDProcessor.processFiles(AbstractPMDProcessor.java:108)
        at net.sourceforge.pmd.PMD.processFiles(PMD.java:329)
        at net.sourceforge.pmd.ant.internal.PMDTaskImpl.doTask(PMDTaskImpl.java:190)
        at net.sourceforge.pmd.ant.internal.PMDTaskImpl.execute(PMDTaskImpl.java:275)
        at net.sourceforge.pmd.ant.PMDTask.execute(PMDTask.java:50)
        at org.apache.tools.ant.UnknownElement.execute(UnknownElement.java:293)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.base/java.lang.reflect.Method.invoke(Method.java:566)
        at org.apache.tools.ant.dispatch.DispatchUtils.execute(DispatchUtils.java:106)
        at groovy.util.AntBuilder.performTask(AntBuilder.java:338)
        at groovy.util.AntBuilder.nodeCompleted(AntBuilder.java:283)
        at org.gradle.api.internal.project.ant.BasicAntBuilder.nodeCompleted(BasicAntBuilder.java:78)
        at jdk.internal.reflect.GeneratedMethodAccessor1888.invoke(Unknown Source)
        at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.base/java.lang.reflect.Method.invoke(Method.java:566)
        at org.codehaus.groovy.reflection.CachedMethod.invoke(CachedMethod.java:98)
        at groovy.lang.MetaMethod.doMethodInvoke(MetaMethod.java:325)
        at org.gradle.internal.metaobject.BeanDynamicObject$MetaClassAdapter.invokeMethod(BeanDynamicObject.java:479)
        at org.gradle.internal.metaobject.BeanDynamicObject.tryInvokeMethod(BeanDynamicObject.java:191)
        at org.gradle.internal.metaobject.AbstractDynamicObject.invokeMethod(AbstractDynamicObject.java:160)
        at org.gradle.api.internal.project.antbuilder.AntBuilderDelegate.nodeCompleted(AntBuilderDelegate.java:118)
        at groovy.util.BuilderSupport.doInvokeMethod(BuilderSupport.java:154)
        at groovy.util.BuilderSupport.invokeMethod(BuilderSupport.java:67)
        at org.codehaus.groovy.runtime.callsite.PogoMetaClassSite.call(PogoMetaClassSite.java:47)
        at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:136)
        at org.gradle.api.plugins.quality.internal.PmdInvoker$_invoke_closure2.doCall(PmdInvoker.groovy:62)
        at jdk.internal.reflect.GeneratedMethodAccessor283.invoke(Unknown Source)
        at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.base/java.lang.reflect.Method.invoke(Method.java:566)
        at org.codehaus.groovy.reflection.CachedMethod.invoke(CachedMethod.java:98)
        at groovy.lang.MetaMethod.doMethodInvoke(MetaMethod.java:325)
        at org.codehaus.groovy.runtime.metaclass.ClosureMetaClass.invokeMethod(ClosureMetaClass.java:264)
        at groovy.lang.MetaClassImpl.invokeMethod(MetaClassImpl.java:1034)
        at groovy.lang.Closure.call(Closure.java:418)
        at groovy.lang.Closure.call(Closure.java:434)
        at org.gradle.api.internal.ClosureBackedAction.execute(ClosureBackedAction.java:71)
        at org.gradle.api.internal.ClosureBackedAction.execute(ClosureBackedAction.java:53)
        at org.gradle.api.internal.project.antbuilder.DefaultIsolatedAntBuilder$2.execute(DefaultIsolatedAntBuilder.java:152)
        at org.gradle.api.internal.project.antbuilder.DefaultIsolatedAntBuilder$2.execute(DefaultIsolatedAntBuilder.java:134)
        at org.gradle.api.internal.project.antbuilder.ClassPathToClassLoaderCache.withCachedClassLoader(ClassPathToClassLoaderCache.java:134)
        at org.gradle.api.internal.project.antbuilder.DefaultIsolatedAntBuilder.execute(DefaultIsolatedAntBuilder.java:128)
        at org.gradle.api.internal.project.IsolatedAntBuilder$execute$0.call(Unknown Source)
        at org.gradle.api.plugins.quality.internal.PmdInvoker.invoke(PmdInvoker.groovy:60)
        at org.gradle.api.plugins.quality.Pmd.run(Pmd.java:93)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.base/java.lang.reflect.Method.invoke(Method.java:566)
        at org.gradle.internal.reflect.JavaMethod.invoke(JavaMethod.java:73)
        at org.gradle.api.internal.project.taskfactory.StandardTaskAction.doExecute(StandardTaskAction.java:46)
        at org.gradle.api.internal.project.taskfactory.StandardTaskAction.execute(StandardTaskAction.java:39)
        at org.gradle.api.internal.project.taskfactory.StandardTaskAction.execute(StandardTaskAction.java:26)
        at org.gradle.api.internal.AbstractTask$TaskActionWrapper.execute(AbstractTask.java:801)
        at org.gradle.api.internal.AbstractTask$TaskActionWrapper.execute(AbstractTask.java:768)
        at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter$1.run(ExecuteActionsTaskExecuter.java:131)
        at org.gradle.internal.operations.DefaultBuildOperationExecutor$RunnableBuildOperationWorker.execute(DefaultBuildOperationExecutor.java:300)
        at org.gradle.internal.operations.DefaultBuildOperationExecutor$RunnableBuildOperationWorker.execute(DefaultBuildOperationExecutor.java:292)
        at org.gradle.internal.operations.DefaultBuildOperationExecutor.execute(DefaultBuildOperationExecutor.java:174)
        at org.gradle.internal.operations.DefaultBuildOperationExecutor.run(DefaultBuildOperationExecutor.java:90)
        at org.gradle.internal.operations.DelegatingBuildOperationExecutor.run(DelegatingBuildOperationExecutor.java:31)
        at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter.executeAction(ExecuteActionsTaskExecuter.java:120)
        at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter.executeActions(ExecuteActionsTaskExecuter.java:99)
        at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter.execute(ExecuteActionsTaskExecuter.java:77)
        at org.gradle.api.internal.tasks.execution.OutputDirectoryCreatingTaskExecuter.execute(OutputDirectoryCreatingTaskExecuter.java:51)
        at org.gradle.api.internal.tasks.execution.SkipUpToDateTaskExecuter.execute(SkipUpToDateTaskExecuter.java:59)
        at org.gradle.api.internal.tasks.execution.ResolveTaskOutputCachingStateExecuter.execute(ResolveTaskOutputCachingStateExecuter.java:54)
        at org.gradle.api.internal.tasks.execution.ValidatingTaskExecuter.execute(ValidatingTaskExecuter.java:59)
        at org.gradle.api.internal.tasks.execution.SkipEmptySourceFilesTaskExecuter.execute(SkipEmptySourceFilesTaskExecuter.java:101)
        at org.gradle.api.internal.tasks.execution.FinalizeInputFilePropertiesTaskExecuter.execute(FinalizeInputFilePropertiesTaskExecuter.java:44)
        at org.gradle.api.internal.tasks.execution.CleanupStaleOutputsExecuter.execute(CleanupStaleOutputsExecuter.java:91)
        at org.gradle.api.internal.tasks.execution.ResolveTaskArtifactStateTaskExecuter.execute(ResolveTaskArtifactStateTaskExecuter.java:62)
        at org.gradle.api.internal.tasks.execution.SkipTaskWithNoActionsExecuter.execute(SkipTaskWithNoActionsExecuter.java:59)
        at org.gradle.api.internal.tasks.execution.SkipOnlyIfTaskExecuter.execute(SkipOnlyIfTaskExecuter.java:54)
        at org.gradle.api.internal.tasks.execution.ExecuteAtMostOnceTaskExecuter.execute(ExecuteAtMostOnceTaskExecuter.java:43)
        at org.gradle.api.internal.tasks.execution.CatchExceptionTaskExecuter.execute(CatchExceptionTaskExecuter.java:34)
        at org.gradle.api.internal.tasks.execution.EventFiringTaskExecuter$1.run(EventFiringTaskExecuter.java:51)
        at org.gradle.internal.operations.DefaultBuildOperationExecutor$RunnableBuildOperationWorker.execute(DefaultBuildOperationExecutor.java:300)
        at org.gradle.internal.operations.DefaultBuildOperationExecutor$RunnableBuildOperationWorker.execute(DefaultBuildOperationExecutor.java:292)
        at org.gradle.internal.operations.DefaultBuildOperationExecutor.execute(DefaultBuildOperationExecutor.java:174)
        at org.gradle.internal.operations.DefaultBuildOperationExecutor.run(DefaultBuildOperationExecutor.java:90)
        at org.gradle.internal.operations.DelegatingBuildOperationExecutor.run(DelegatingBuildOperationExecutor.java:31)
        at org.gradle.api.internal.tasks.execution.EventFiringTaskExecuter.execute(EventFiringTaskExecuter.java:46)
        at org.gradle.execution.taskgraph.LocalTaskInfoExecutor.execute(LocalTaskInfoExecutor.java:42)
        at org.gradle.execution.taskgraph.DefaultTaskExecutionGraph$BuildOperationAwareWorkItemExecutor.execute(DefaultTaskExecutionGraph.java:277)
        at org.gradle.execution.taskgraph.DefaultTaskExecutionGraph$BuildOperationAwareWorkItemExecutor.execute(DefaultTaskExecutionGraph.java:262)
        at org.gradle.execution.taskgraph.DefaultTaskPlanExecutor$ExecutorWorker$1.execute(DefaultTaskPlanExecutor.java:135)
        at org.gradle.execution.taskgraph.DefaultTaskPlanExecutor$ExecutorWorker$1.execute(DefaultTaskPlanExecutor.java:130)
        at org.gradle.execution.taskgraph.DefaultTaskPlanExecutor$ExecutorWorker.execute(DefaultTaskPlanExecutor.java:200)
        at org.gradle.execution.taskgraph.DefaultTaskPlanExecutor$ExecutorWorker.executeWithWork(DefaultTaskPlanExecutor.java:191)
        at org.gradle.execution.taskgraph.DefaultTaskPlanExecutor$ExecutorWorker.run(DefaultTaskPlanExecutor.java:130)
        at org.gradle.internal.concurrent.ExecutorPolicy$CatchAndRecordFailures.onExecute(ExecutorPolicy.java:63)
        at org.gradle.internal.concurrent.ManagedExecutorImpl$1.run(ManagedExecutorImpl.java:46)
        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
        at org.gradle.internal.concurrent.ThreadFactoryImpl$ManagedThreadRunnable.run(ThreadFactoryImpl.java:55)
        at java.base/java.lang.Thread.run(Thread.java:834)
Caused by: org.jaxen.XPathSyntaxException: Expected: )
        at org.jaxen.BaseXPath.<init>(BaseXPath.java:121)
        at org.jaxen.BaseXPath.<init>(BaseXPath.java:142)
        at net.sourceforge.pmd.lang.ast.AbstractNode.findChildNodesWithXPath(AbstractNode.java:423)
        at net.sourceforge.pmd.lang.ast.AbstractNode.hasDescendantMatchingXPath(AbstractNode.java:430)
        ... 114 more
Caused by: class org.jaxen.saxpath.XPathSyntaxException:
                    .[(ancestor::ClassOrInterfaceDeclaration/@Image = 'DiscordWebhooksDiscordManager')
                        and (@BeginLine = 98)
                        and (@BeginColumn = (17, 60))]"
                : 197: Expected: )
        at org.jaxen.saxpath.base.XPathReader.createSyntaxException(XPathReader.java:1085)
        at org.jaxen.saxpath.base.XPathReader.match(XPathReader.java:1038)
        at org.jaxen.saxpath.base.XPathReader.filterExpr(XPathReader.java:274)
        at org.jaxen.saxpath.base.XPathReader.pathExpr(XPathReader.java:142)
        at org.jaxen.saxpath.base.XPathReader.unionExpr(XPathReader.java:1007)
        at org.jaxen.saxpath.base.XPathReader.unaryExpr(XPathReader.java:995)
        at org.jaxen.saxpath.base.XPathReader.multiplicativeExpr(XPathReader.java:943)
        at org.jaxen.saxpath.base.XPathReader.additiveExpr(XPathReader.java:913)
        at org.jaxen.saxpath.base.XPathReader.relationalExpr(XPathReader.java:860)
        at org.jaxen.saxpath.base.XPathReader.equalityExpr(XPathReader.java:840)
        at org.jaxen.saxpath.base.XPathReader.andExpr(XPathReader.java:809)
        at org.jaxen.saxpath.base.XPathReader.orExpr(XPathReader.java:787)
        at org.jaxen.saxpath.base.XPathReader.expr(XPathReader.java:780)
        at org.jaxen.saxpath.base.XPathReader.filterExpr(XPathReader.java:273)
        at org.jaxen.saxpath.base.XPathReader.pathExpr(XPathReader.java:142)
        at org.jaxen.saxpath.base.XPathReader.unionExpr(XPathReader.java:1007)
        at org.jaxen.saxpath.base.XPathReader.unaryExpr(XPathReader.java:995)
        at org.jaxen.saxpath.base.XPathReader.multiplicativeExpr(XPathReader.java:943)
        at org.jaxen.saxpath.base.XPathReader.additiveExpr(XPathReader.java:913)
        at org.jaxen.saxpath.base.XPathReader.relationalExpr(XPathReader.java:860)
        at org.jaxen.saxpath.base.XPathReader.equalityExpr(XPathReader.java:829)
        at org.jaxen.saxpath.base.XPathReader.andExpr(XPathReader.java:809)
        at org.jaxen.saxpath.base.XPathReader.andExpr(XPathReader.java:819)
        at org.jaxen.saxpath.base.XPathReader.andExpr(XPathReader.java:819)
        at org.jaxen.saxpath.base.XPathReader.orExpr(XPathReader.java:787)
        at org.jaxen.saxpath.base.XPathReader.expr(XPathReader.java:780)
        at org.jaxen.saxpath.base.XPathReader.predicateExpr(XPathReader.java:775)
        at org.jaxen.saxpath.base.XPathReader.predicate(XPathReader.java:766)
        at org.jaxen.saxpath.base.XPathReader.predicates(XPathReader.java:751)
        at org.jaxen.saxpath.base.XPathReader.abbrStep(XPathReader.java:730)
        at org.jaxen.saxpath.base.XPathReader.step(XPathReader.java:514)
        at org.jaxen.saxpath.base.XPathReader.steps(XPathReader.java:442)
        at org.jaxen.saxpath.base.XPathReader.relativeLocationPath(XPathReader.java:426)
        at org.jaxen.saxpath.base.XPathReader.locationPath(XPathReader.java:340)
        at org.jaxen.saxpath.base.XPathReader.pathExpr(XPathReader.java:179)
        at org.jaxen.saxpath.base.XPathReader.unionExpr(XPathReader.java:1007)
        at org.jaxen.saxpath.base.XPathReader.unaryExpr(XPathReader.java:995)
        at org.jaxen.saxpath.base.XPathReader.multiplicativeExpr(XPathReader.java:943)
        at org.jaxen.saxpath.base.XPathReader.additiveExpr(XPathReader.java:913)
        at org.jaxen.saxpath.base.XPathReader.relationalExpr(XPathReader.java:860)
        at org.jaxen.saxpath.base.XPathReader.equalityExpr(XPathReader.java:829)
        at org.jaxen.saxpath.base.XPathReader.andExpr(XPathReader.java:809)
        at org.jaxen.saxpath.base.XPathReader.orExpr(XPathReader.java:787)
        at org.jaxen.saxpath.base.XPathReader.expr(XPathReader.java:780)
        at org.jaxen.saxpath.base.XPathReader.parse(XPathReader.java:100)
        at org.jaxen.BaseXPath.<init>(BaseXPath.java:116)
        ... 117 more
@jsotuyod
Copy link
Member

jsotuyod commented Feb 19, 2019

@Vampire PMD has always supported XPath 1.0, 2.0 and "1.0-compatibility" for writing XPath rules. There is nothing new here.

This property however, has always been treated as an XPath 1.0 expression (again, nothing changed).

I agree however, that as we push towards dropping 1.0 eventually (and even adding 3.0 support), we need to have such support across the board. Without adding an extra property, I see no reasonable way of doing this however… probably @adangel or @oowekyala have better ideas 'though

@jsotuyod jsotuyod added the an:enhancement An improvement on existing features / rules label Feb 19, 2019
@Vampire
Copy link
Contributor Author

Vampire commented Feb 20, 2019

I see all three modes in the designer, except for it 2.0 and 1.0-compatibility not working as mentioned in the other issue.
Ah, right, XPath is not backwards compatible, I forgot.
But maybe the documentation at https://pmd.github.io/pmd-6.11.0/pmd_userdocs_suppressing_warnings.html#the-property-violationsuppressxpath then should mention that only XPath 1.0 is supported?
Currently it only says "XPath", not which version.

As 2.0 is not backwards compatible and expressions that are syntactically valid in both, 1.0 and 2.0 could behave quite differently (e. g. "4" > "4.0" which will be a numerical comparison in 1.0 but a string comparison in 2.0) I also see only the way to make it explicit which version to use. You could use differently named properties, or some marker as prefix in the value. Or maybe you could use the type argument of the property tag? According to the XSD it is a string attribute, so you should be able to use any value there.

@oowekyala
Copy link
Member

The type attribute is made for declaring properties, it can't be used when overriding a value. Scheduled schema changes for 7.0.0 will make that clearer and forbid its use in that context (details in wiki the wiki).

Alternatively we could stop exposing that violationSuppressXPath and violationSuppressRegex are properties in the XML and instead introduce some syntactic sugar with 7.0.0. I think it makes sense, just like we're introducing some sugar for Xpath rule properties. E.g.

<suppress-xpath version="2.0">
  ...

@Vampire
Copy link
Contributor Author

Vampire commented Feb 20, 2019

Also fine. :-)
What sugar is available for rule properties?

@oowekyala
Copy link
Member

oowekyala commented Feb 20, 2019

We're considering making XPath rule declarations shorter by specializing the rule element. Basically it becomes impossible to set the class explicitly (it's always XPathRule anyway), allowing language modules to select a specialized implementation. The xpath expression won't be entered by a regular property element but a special xpath element which will be required. For example:

    <xpath-rule name="Name" language="java" message="string">
        <description>
            A description
        </description>

        <xpath><![CDATA[

            //ClassOrInterfaceDeclaration
              [@Abstract='true' and @Interface='false']

        ]]><xpath>
    </xpath-rule>

Doing that has other pleasing consequences that are described on the wiki page I linked. This is a change considered for 7.0.0 and nothing is implemented yet.

Anyway it's quite natural to add an element for suppression patterns. They'd hide the implementation, enable IDE completion and inline doc, etc.

@Vampire
Copy link
Contributor Author

Vampire commented Feb 20, 2019

Ah, I see, I thought there might be some sugar for using the xpath rules that I missed to use.

Sounds reasonable though.
Also you could maybe hint IntelliJ that the content of xpath and suppress-xpath are XPath, then you get syntax highlighting and so on.
This can also be done manually, but would be nice of course if this worked out of the box if possible.
Currently now all my <value> tag contents are highlighted as XPath because it cannot differentiate on the name of the parent tag when injecting a language. :-)

@Vampire
Copy link
Contributor Author

Vampire commented Feb 20, 2019

Oh, you even can add conditions, so you can restrict the injection with an XPath condition, I just don't get it working properly if it is not broken currently (https://youtrack.jetbrains.com/issue/IDEA-207620).

@oowekyala
Copy link
Member

Also you could maybe hint IntelliJ that the content of xpath and suppress-xpath are XPath, then you get syntax highlighting and so on.

We don't maintain the intellij plugin, submit a feature request on https://github.com/amitdev/PMD-Intellij for that. Not sure how responsive they are though

@Vampire
Copy link
Contributor Author

Vampire commented Feb 20, 2019

I did't mean in the IJ plugin, but in the XSD if this is possible, I didn't find anything though, I created https://youtrack.jetbrains.com/issue/IDEA-207628 as feature request.
If you e. g. develop a library and you have a method that takes some XPath expression as String parameter, you can use Jetbrains annotation @Language on the parameter to define that XPath should be injected, so anyone using the lib in IJ automatically gets the highlighting. I hoped you can somehow also define this in an XSD, but I at least didn't find something.

@oowekyala oowekyala changed the title [core] XPath version 2 not properly supported [core] Select XPath version in violationSuppressXPath Feb 21, 2019
@oowekyala oowekyala added the in:xpath Relating to xpath support at large, eg Jaxen / Saxon, custom functions, attribute resolution label Feb 21, 2019
@oowekyala
Copy link
Member

Ok so we need a transition plan for this on master that fits within #1687. The current problem is

  • XPath 1.0 support is deprecated and will be removed on 7.0, rules are already migrated
  • violationSuppressXPath only supports XPath 1.0, and there's no early transition path before 7.0. When migrating to 7.0 your suppression xpath will just be broken and you'll have to update them in bulk at the time you upgrade to 7.0, you can't do that gradually and be ready when 7.0 comes out

Additionally, this doesn't need to be implemented via a property, and some sugar would be nice.

I'm thinking, in order to support early migration, we could just make a decision about what the new XML syntax is to suppress violations via XPath, and then make that new syntax available on master, but only working with XPath 2. Eg if we add a child element to <rule>:

<rule ref="...">
  <suppressions>
     <xpath>./Some/XPath2</xpath>
  </suppressions>
</rule>

Where the version of this query would be 2.0 by defaut. So, users can migrate by transforming

<properties>
  <property name="violationSuppressXPath" value="./Some/XPath1" />
</properties>

to the above syntax, and migrating their xpath query to 2.0 at the same time

The advantage of a dedicated <suppressions> element are multiple:

  • you can mention several <xpath> queries instead of cramming all your suppressions into the same query
  • the <suppressions> element can also have <messageRegex> children, which replace violationSuppressRegex
  • the element is open for future extensions, eg <filepath> suppressions or whatever
    • by extension, the xpath element is open to be extended with a version attribute, should we require it in the future. This is the problem with current properties
  • the fact that those internally use properties is an implementation detail that is hidden by this syntax, in PMD 7 we'll have our hands free to implement this in another way if useful
  • it's much more apparent at a glance what these expressions are doing
  • an updated schema would mention suppressions and the possible children, meaning you'd have IDE autocompletion for it. The schema could also use xs:documentation to point users to our documentation page.

@oowekyala
Copy link
Member

At this point I don't think it's worth expending the effort of doing this change to the schema in PMD 6. The incompatibilities between XPath 1 and 2 are few and far between. XPath expressions will be broken by the new grammar in PMD 7 anyway.

When releasing PMD 7, we can use the XSLT migrator to convert the property to the new element form, and then point the user to the elements that need to be reviewed, because they've probably been invalidated by changes to the grammar.

The changes to the schema can be scheduled for PMD 7 (I'll copy this comment to the wiki).

In the meantime, we could also decide to make violationSuppressXPath use 2.0 in some minor PMD 6 release. This sounds like a breaking change, but most people won't see a difference. The ones who do can be referred to the migration guide, and this would fix this ticket.

@oowekyala oowekyala added the in:ruleset-xml About the ruleset schema/parser label Nov 12, 2020
@linusjf

This comment has been minimized.

@linusjf

This comment has been minimized.

@oowekyala

This comment has been minimized.

@adangel
Copy link
Member

adangel commented Jan 11, 2023

Moving the comments from #2523 to here:

  • Note 1: with [core] Upgrade Saxon, add XPath 3.1, remove Jaxen #2614, violationSuppressXPath will use the default XPath version and use Saxon. That's a change in PMD 7.
  • Note 2: ViolationSuppressor: xpath suppressions handling. The parsed xpath expr should be stored instead of the string
  • There will be a intermediate rule set schema, that supports both old way of declaring suppressions via violationSuppressXPath / violationSuppressRegex and the new way with explicit elements (suppressions/xpath und suppressions/regex).
  • Not sure this is necessary to do on master, since incompatibilities between the two xpath versions are few.
  • For PMD 7, we'll should additionally provide a cleaned schema (not sure yet, whether that's neccessary).
  • Note: This also depends on PMD 7.0.0 API - ruleset schema

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
an:enhancement An improvement on existing features / rules in:ruleset-xml About the ruleset schema/parser in:xpath Relating to xpath support at large, eg Jaxen / Saxon, custom functions, attribute resolution
Projects
None yet
Development

No branches or pull requests

5 participants