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.

Sign up for GitHub

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

Jump to bottom

[core] Text documents #3893

Merged
merged 240 commits into from
Jul 17, 2022
Merged

[core] Text documents #3893

merged 240 commits into from
Jul 17, 2022

Conversation

oowekyala
Copy link
Member

@oowekyala oowekyala commented Apr 1, 2022 •
edited
Loading

Describe the PR

(this now contains #3892)

Changelog

  • pmd-apex

    • Text position handling is simplified (SourceCodePositioner is now part of TextDocument)
    • The Location attribute is removed
    • EmptyCatchBlock now considers doc comments as regular comments
    • Some nodes now have a different report location: eg for methods and classes it's just their identifier, like for pmd-java, ref [java] Single Line Warnings #730

  • pmd-core

    • New API: Chars, a string slice type. Supports efficient string-like operations without copying the char array of the file. Makes comment handling more efficient, in a future PR, will be used by Javacc tokens.
    • TextFile is improved
      • Line terminators are normalized to \n
      • Reading the file computes a checksum which is used by incremental analysis later
      • TextFile replaces DataSource everywhere in the internals of PMD
    • New API: TextDocument, built on top of TextFile
    • New API: FileLocation. This represents the line/column coordinates of a place in a TextDocument. Used for reporting.
    • New API: Reportable. This is an interface for anything that can provide a FileLocation (Nodes and Tokens currently)
    • New API: TextRegion. This represents a range of text in a TextDocument with offset/length, which is useful for tokens and for TextAvailableNodes in general
    • changes to Node interface:
      • getTextRegion is a new abstract method. getReportLocation is not abstract anymore. TextAvailableNode refines the contract of getTextRegion to mean that the text region is the exact boundaries of the node in the text, which is eg not the case for the apex module, but is the case for Javacc-based modules.

  • Some usage of Path and File are replaced with String in eg Ruleset#applies (file name filters) because TextFile uses strings as path names.

  • CPD

    • CPD uses FileLocation and TextDocument
    • The end goal is to replace SourceCode with TextDocument and TextFile
    • This refactoring is incomplete for now, some transitional APIs are in the class CpdCompat. Removing them requires [core] Merge CPD and PMD language #3919
    • Antlr tokens are now easier to build because they can use a TextDocument

  • pmd-java

  • pmd-lang-test

    • Things to represent and test text locations are replaced with standard pmd-core APIs (FileLocation/TextRegion)

  • pmd-test

    • Change to how the "code" element is trimmed, before it was just String::trim() which doesn't preserve indent on the first line. Now it uses basically a clone of String::stripIndent (method used by text blocks in JDK 16+) so that the code behaves like a text block (without escapes)

  • pmd-cpp

    • The "skip block" feature is temporarily dysfunctional, it's fixed in the followup branch, I just ignored the tests

Future refactorings:

  • violation decorators are needed to remove RuleViolationFactory
  • finish the CPD refactoring (SourceCode -> TextDocument)
  • integrate TextDocument more cleanly within our javacc support

Related issues

Ready?

  • Added unit tests for fixed bug/feature
  • Passing all unit tests
  • Complete build ./mvnw clean verify passes (checked automatically by github actions)
  • Added (in-code) documentation (if needed)
  • Investigate required changes in the designer -> Compat with pmd/pmd#3893 pmd-designer#51

@oowekyala
Improve SourceCodePositioner
271121c
@oowekyala
Improve document logic
d10b2b6
@oowekyala
Cleanup tests
2885a45
@oowekyala
Cleanup api
7705439
@oowekyala
Fix closing
1125fb1
@oowekyala
Always check bounds of regions
d5d30dc
@oowekyala
Remove text operations
1d7f964
@oowekyala
Cleanup
a733da4
@oowekyala
Split impl into two classes
5e48926
@oowekyala
Prefer offset-based repr to lineCol
b127808
@oowekyala
Rename to avoid conflict with dom API
a79f984
@oowekyala
Checkstyle
add6cf8
@oowekyala
Test regions
7d880a0
@oowekyala
Remove insert from line+col
e89e35c
@oowekyala
Use TextEditor instead
6eb44a7
@oowekyala
Extract buffering logic
8174e46
@oowekyala
Remove create region by line+col
44c9140
@oowekyala
Specification
52927aa
@oowekyala
Detect overlap wip
072ac21
@oowekyala
Rename TextFile
f4458e5
@oowekyala
Test text region
28a1ce0
@oowekyala
Document source code positioner
2b62c47
@oowekyala
Catch other case of overlap
3a7e0c0
@oowekyala
Doc stringUtil addition
97e891b
@oowekyala
Create SourceCodePositioner lazily
4584071
@oowekyala
Remaining tests for TextRegion
b4d72e6
@oowekyala
Add data source adapter
3b96e9e
@oowekyala
Remove custom annots
709b197
@oowekyala
Doc
b958b12
@oowekyala
Add close semantics
9ca01cd 
Remove data source adapter
@adangel adangel mentioned this pull request May 5, 2022
Closed
adangel
adangel approved these changes May 5, 2022
View reviewed changes
Copy link
Member

@adangel adangel left a comment

Choose a reason for hiding this comment

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

I leave merging this up to you.
Thanks for the work!

@adangel
Copy link
Member

adangel commented May 6, 2022

Ok, the regression tester ran now... but with some more work to do: It seems, the reported file names are now different.... all violations have been removed and added.

It's easier to look at a smaller project, like https://chunk.io/pmd/23054915090a48adb1f094dcfc0e981a/diff1/Schedul-o-matic-9000/index.html

The xml report (baseline): https://chunk.io/pmd/23054915090a48adb1f094dcfc0e981a/diff1/Schedul-o-matic-9000/base_pmd_report.xml

<file name="/home/runner/work/pmd/target/repositories/Schedul-o-matic-9000/force-app/main/default/classes/Dao.cls">
<violation beginline="8" endline="8" begincolumn="14" endcolumn="16" rule="ApexSharingViolations" ruleset="Security" externalInfoUrl="https://pmd.github.io/pmd/pmd_rules_apex_security.html#apexsharingviolations" priority="3">
Apex classes should declare a sharing model if DML or SOQL/SOSL is used
</violation>
...

The xml report (patch): https://chunk.io/pmd/23054915090a48adb1f094dcfc0e981a/diff1/Schedul-o-matic-9000/patch_pmd_report.xml

<file name="target/repositories/Schedul-o-matic-9000/force-app/main/default/classes/Dao.cls">
<violation beginline="8" endline="8" begincolumn="14" endcolumn="17" rule="ApexSharingViolations" ruleset="Security" externalInfoUrl="https://pmd.github.io/pmd/pmd_rules_apex_security.html#apexsharingviolations" priority="3">
Apex classes should declare a sharing model if DML or SOQL/SOSL is used
</violation>
...

The filenames are now relativized already... but only to the current working directory (probably).

@oowekyala
Copy link
Member Author

oowekyala commented May 6, 2022 •
edited
Loading

Thanks for looking into this Andreas. One thing to keep in mind is that this PR changes the implementation of the "short file name" functionality. The TextFiles are all created with a "display name", which is relativized if the short file name option is enabled, but it's not absolutized otherwise -> this is probably the reason for the difference. So the display name is absolute only if you use an absolute path as a CLI argument, which the regression tester is probably not doing.

I think this is sensible behaviour (at least it's consistent), but maybe we should think more about this... I think the good thing about this is that the report is maybe more portable. But some renderers might mandate relative or absolute paths, like #3798 shows

@adangel
Copy link
Member

adangel commented May 6, 2022

I think, both behaviors make sense. But since we are on PMD 7, we can change it...

I think the good thing about this is that the report is maybe more portable.

👍

Hm... theoretically, we don't need to change the regression tester. It would work after this PR is merged, then the baseline also has the same paths in the report...

@oowekyala oowekyala mentioned this pull request May 7, 2022
Merged
4 tasks
@oowekyala oowekyala mentioned this pull request Jul 10, 2022
Closed
@oowekyala oowekyala force-pushed the text-utils-simple branch from 70d3a40 to df4dc68 Compare July 16, 2022 18:46
@oowekyala oowekyala force-pushed the text-utils-simple branch from df4dc68 to 0057492 Compare July 16, 2022 20:16
@oowekyala oowekyala marked this pull request as ready for review July 17, 2022 11:02
@oowekyala oowekyala merged commit 451cbb6 into pmd:pmd/7.0.x Jul 17, 2022
@oowekyala oowekyala deleted the text-utils-simple branch July 18, 2022 10:44
@adangel adangel mentioned this pull request Jan 23, 2023
Closed
55 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adangel adangel adangel approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@oowekyala @adangel