Skip to content

[core] Upgrade Saxon, add XPath 3.1, remove Jaxen#2614

Merged
oowekyala merged 55 commits into
pmd:pmd/7.0.xfrom
oowekyala:update-saxon-version
Aug 23, 2020
Merged

[core] Upgrade Saxon, add XPath 3.1, remove Jaxen#2614
oowekyala merged 55 commits into
pmd:pmd/7.0.xfrom
oowekyala:update-saxon-version

Conversation

@oowekyala

@oowekyala oowekyala commented Jun 25, 2020

Copy link
Copy Markdown
Member

Part of #2523

I don't think this is completely ready, but I'm putting this up for later This is ready now, the sooner it's merged the better

Describe the PR

  • Upgrade saxon to Saxon HE 10.1
    • This means, XPath version 1 and 1.0 compatibility are removed
    • The default XPath version is now 3.1, which didn't entail changes to rules
  • Remove Jaxen
    • This includes removing XPathRuleQuery, because there's a single implementation
    • This also makes violationSuppressXPath use XPath 3.1
  • Move most of the XPath classes into package nspmd.lang.rule.xpath. This includes class Attribute, which some clients may depend on as it's exposed on Node.
    • TODO mention this in the release notes, in the wiki about API maybe
    • For completeness, XPathRule could be moved into this package. But it's very depended on so I'm not sure
  • Remove Node::hasDescendantMatchingXPath by updating the last images
    • I gave up on updating usages of Node::findChildrenWithXPath for now. I think it would be much easier to scrap that after the new java grammar is merged. The method now uses Saxon, which caused no problems

Also one new XPath optimisation, is that rules property values are now inlined in the xpath expression. This allows pruning some branches that would never be taken, because some property is false.

Related issues

Ready?

  • Test XPath 3.1
  • Documentation on the website
  • Release notes:
    • Mention moved API
    • Mention version change, document new XPath 3.1 features
  • Added unit tests for fixed bug/feature
  • Passing all unit tests
  • Complete build ./mvnw clean verify passes (checked automatically by travis)
  • Added (in-code) documentation (if needed)

Remove Jaxen, port function defs

Use enum to represent XPath version

Move to internal package

Fix style

Refactor functions
LazyExpression does not exist anymore
Saxon HE doesn't implement typed attributes,
I guess that's part of the package of schema
aware processing :( We could possibly replace
all AttributeGetter exprs with TypedAttributeGetter
or something, but this will be very hard as there
is no visitor pattern or cloning pattern implemented
widely in saxon (Expression has 166 implementations).
 We could literally replace the class for
AttributeGetter in the saxon jar, provided it's legal
The RootNode corresponds to both an element
and a document node
This reverts commit 3d463d7.

This can be done later.
Not as stable as 8, but fixes a
bug in the optimizer
@oowekyala oowekyala added the in:xpath Relating to xpath support at large, eg Jaxen / Saxon, custom functions, attribute resolution label Jun 25, 2020
@oowekyala oowekyala added this to the 7.0.0 milestone Jun 25, 2020
@ghost

ghost commented Jun 25, 2020

Copy link
Copy Markdown
1 Message
📖 No java rules are changed!

Generated by 🚫 Danger

@adangel adangel mentioned this pull request Jul 19, 2020
14 tasks
@adangel adangel added the is:WIP For PRs that are not fully ready, or issues that are actively being tackled label Jul 19, 2020
@oowekyala oowekyala added is:WIP For PRs that are not fully ready, or issues that are actively being tackled and removed is:WIP For PRs that are not fully ready, or issues that are actively being tackled labels Aug 8, 2020
@oowekyala oowekyala removed the is:WIP For PRs that are not fully ready, or issues that are actively being tackled label Aug 10, 2020

@adangel adangel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this! Looks good.

This PR actually fixes #1687, doesn't it?

I've collected the following comments, which I added to #2523:

  • [doc] Make sure, XPath changes are documented in a migration guide PMD 6->7 (see also #1139)

    That means: The section migrating from XPath 1.0 -> XPath 2.0 should be moved to the migration
    guide PMD6->7
    since only XPath 2.0 (actually XPath 3.1) will be supported with PMD 7.

  • Move XPathRule to correct package. For compatibility, we could keep a @DeprecatedUntilPmd700
    subclass in place. Depends on the final ruleset xml format.

  • Cleanup duplicated license headers in files. Appeared after moving files around.

  • [api] AbstractXPathFunctionDef/XPathHandler -> uses saxon api and leaks this into pmd api for
    language implementations

  • [deprecations] The property version of XPathRule is deprecated and will be removed.
    -> eventually, we should remove it in pmd7, don't we? hm... it's not deprecated in pmd6, right?
    because you can still switch between 1.0, 1.0-compat and 2.0. but maybe we can deprecated in
    already in pmd6, defaulting to xpath 2.0, then we could remove it in pmd7?
    -> it seems, the version property is already deprecated on pmd6

  • ViolationSuppressor: xpath suppressions handling. The parsed xpath expr should be stored
    instead of the string.

@oowekyala

Copy link
Copy Markdown
Member Author

[deprecations] The property version of XPathRule is deprecated and will be removed.
-> eventually, we should remove it in pmd7, don't we? hm... it's not deprecated in pmd6, right?
because you can still switch between 1.0, 1.0-compat and 2.0. but maybe we can deprecated in
already in pmd6, defaulting to xpath 2.0, then we could remove it in pmd7?
-> it seems, the version property is already deprecated on pmd6

It's deprecated on master but I think not as a property (deprecated!), so that people can still upgrade to XPath 2 before 7.0. I think in PMD 7 the reasonable thing would be to remove it, but after we've done changes to the ruleset schema probably, so not before a long time.

Thanks for adding those todos, that's super helpful

@oowekyala oowekyala merged commit d5f48c4 into pmd:pmd/7.0.x Aug 23, 2020
oowekyala added a commit that referenced this pull request Aug 23, 2020
They're transitively gotten from
pmd-core, but for some use cases
this is nicer

eg `mvn install -pl pmd-java`, given
that pmd-core was already installed
locally, should succeed. If the dependencies
are not mentioned explicitly, for some
reason this fails.

Before #2614, pmd-core was shaded with
its dependencies and this wasn't necessary
@oowekyala oowekyala deleted the update-saxon-version branch August 23, 2020 18:50
@adangel adangel changed the title [core] Upgrade Saxon, remove Jaxen [core] Upgrade Saxon, add XPath 3.1, remove Jaxen Jan 11, 2023
@adangel adangel added the dependencies Pull requests that update a dependency file label Jan 12, 2023
@adangel adangel mentioned this pull request Jan 23, 2023
55 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Pull requests that update a dependency file in:xpath Relating to xpath support at large, eg Jaxen / Saxon, custom functions, attribute resolution

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants