Repository navigation
[html] Fix #5313: CPD end line without trailing newline - #7050
Conversation
|
Compared to main: (comment created at 2026-09-29 20:16:20+00:00 for 51762a8) |
b50388a to
1fc2e11
Compare
9c7831f to
cf6cb32
Compare
|
If I understand this PR correctly, this changes the location of the token before each Why does that change if the What does this do, if there is no text directly before the I'm skeptical if faking any locations is the way to go here. If one file ends with a newline and the other doesn't, maybe not reporting the last line is correct? I'd be more interested in finding out why the last line isn't reported if both files don't end with a newline. |
adangel
changed the title
Signed-off-by: Eymen <[email protected]>
cf6cb32 to
b332140
Compare
Signed-off-by: Eymen <[email protected]>
|
Now we have (again, going with |
Signed-off-by: Eymen <[email protected]>
| [h1] 9 13 | ||
| [world] 13 17 | ||
| [/h1] 18 23 |
There was a problem hiding this comment.
The h1 tag used to go from line 5, column 9 to line 5, column 22
Now it goes from line 5, column 9 to line 5, column 23
Do we have an off-by-one-error somewhere?
There was a problem hiding this comment.
Checked this against the source positions and PMD's end-exclusive location convention. The line in question is the separate [/h1] token: starts at column 18 and ends after the > at exclusive column 23, while the > itself is column 22. The opening [h1] token remains 9-13. The focused HtmlCpdLexerTest passes 9/9, so I don't think this is an off-by-one.
There was a problem hiding this comment.
You're right. The new positions are correct. Looks like the old values were wrong.
|
@adangel This PR completely changes how we do tokenization of HTML. Is this ok for you? I never liked how we had tokens nested within tokens for HTML, so I like it. But if this change is too much for a minor version, please stop me before I merge this. |
adangel
left a comment
adangel
left a comment
There was a problem hiding this comment.
Looks good, thanks!
@adangel This PR completely changes how we do tokenization of HTML. Is this ok for you?
I never liked how we had tokens nested within tokens for HTML, so I like it. But if this change is too much for a minor version, please stop me before I merge this.
I'd consider the actual tokenization an implementation detail, so I think this is fine. The important point is, that CPD can correctly detect duplicates, and I think, this still works. It might of course now change, what exactly it detects at duplicates (the whole point of the bugfix).
So, yes, go ahead and merge it :)
I agree, that the new way makes more sense, to have the end tags recorded as individual tokens. And the endcolumn of the start tags was obviously the endcolumn of the closing tag (which might have been on a different line anyway). So, it was a real bug.
One thing, that's still wrong (Could be done with a separate PR for completeness): For text nodes, we report the wrong endcolumn. The endcolumn should be exclusive, but e.g. "hello" in L3 in SimpleHtmlFile.txt should go from columns 11-16.
Describe the PR
Uses Jsoup's tracked source ranges for HTML CPD tokens. Element tokens cover their opening tags, while explicit closing tags are emitted as separate post-order tokens at their own source ranges. Implicit, self-closing, and void elements do not get synthetic closing tokens.
This makes duplicate ranges include the final explicit closing tag when inputs lack trailing newlines, including mixed trailing-newline inputs, without extending unrelated token locations.
Related issues
Ready?
./mvnw clean verifypasses (checked automatically by github actions)Tests
./mvnw -pl pmd-html -am verify./mvnw -pl pmd-html verify