Issue #13809: Kill mutation in CommonUtil #13953

Kevin222004 · 2023-10-26T20:20:46Z

Issue #13809: Kill mutation in CommonUtil

regression is taken from #13284
gist: https://gist.github.com/romani/f15f3afabac0f9ad1e63f429b62d445f

Diff Regression config: https://gist.githubusercontent.com/romani/f15f3afabac0f9ad1e63f429b62d445f/raw/40dfb3e50dc3e5606844f00d074f572be4682079/Config.xml
Diff Regression projects: https://gist.githubusercontent.com/Kevin222004/9600f179b602d4c971bdb0a050099005/raw/360a95ed7bb60d7a0956e531199d484c4d6f6617/test-projects.properties
Report label: Regression-with-path-for-basedir

romani

Items :

romani · 2023-10-27T02:41:58Z

src/main/java/com/puppycrawl/tools/checkstyle/utils/CommonUtil.java

@@ -261,8 +261,8 @@ public static String relativizeAndNormalizePath(final String baseDirectory, fina
            resultPath = path;
        }
        else {
-            final Path pathAbsolute = Paths.get(path).normalize();
-            final Path pathBase = Paths.get(baseDirectory).normalize();
+            final Path pathAbsolute = Paths.get(path);


Looks like we need to test message of violation and validate how path is starting https://www.google.com/amp/s/www.geeksforgeeks.org/path-normalize-method-in-java-with-examples/amp/

To be OS agnostic we usually avoid checking full path in violation, we might do only file name

What we can test is ... We set path with . and/or .. but have in violation path without such symbols.

I have tried multiple ways but at the i didn't find a way in which relativize path is getting affected.

yes, such normalization is done by jdk
https://github.com/openjdk/jdk/blob/db3402577a2c14a41045753a1ffe2829a6bdda91/src/java.base/unix/classes/sun/nio/fs/UnixPath.java#L478-L495

we already have test on this

checkstyle/src/test/java/com/puppycrawl/tools/checkstyle/utils/CommonUtilTest.java

Lines 221 to 224 in bd3a9bb

public void testRelativeNormalizedPathWithDenormalizedBaseDirectory() throws IOException {

final String sampleAbsolutePath = new File("src/main/java").getCanonicalPath();

final String absoluteFilePath = sampleAbsolutePath + "/SampleFile.java";

final String basePath = sampleAbsolutePath + PATH_DENORMALIZER;

We do not need to care about path in zips and JrtPath is something I do not understand.

We do not need to care about path in zips

You may want to recheck this. Some users put configuration files into external JARs controlled by Maven. Those are basically ZIP files.

We will happily fix issue if they will show how to reproduce issue.

It will be easy, revert with already known input how to reproduce it.

romani

ok to merge

rnveach · 2023-10-29T14:27:20Z

Where is regression showing this won't be an issue? This is used by Checker.
This method is not really called "relativizeAndNormalizePath" if there is no more normalize.

romani · 2023-10-29T14:56:59Z

Where is regression showing this won't be an issue? This is used by Checker.

regression is only covering behavior on config, to make this regression something weird should be done with paths to validate. non of our tools can do this.

This method is not really called "relativizeAndNormalizePath" if there is no more normalize.

jdk relativize does normalize internaly. Do you propose to change name of util method ?

rnveach · 2023-10-29T15:01:12Z

Regression should also cover file paths which is done by Checker, as 2 paths need to line up for a match for differencing. If it truthfully has no impact, regression will show no differences either way and will show we won't have anything to worry about after merge.

I am leaning to changing the util name and javadoc since I assume it was named normalize for the normalize that happened in it.

romani · 2023-10-29T17:50:14Z

method is renamed.

romani · 2023-10-29T17:54:42Z

Github, generate report

github-actions · 2023-10-29T20:42:11Z

Regression-1:
https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/d9c038b_2023203029/reports/diff/index.html

romani · 2023-10-29T20:55:03Z

@rnveach , please review.

rnveach · 2023-10-29T21:26:29Z

@romani Please make sure you are using a config to hit the code. baseDir has to be set to trigger the changed code as shown in the method in this pr.

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/utils/CommonUtil.java

Lines 260 to 265 in c6a59c8

    
           if (baseDirectory == null) { 
        
               resultPath = path; 
        
           } 
        
           else { 
        
               final Path pathAbsolute = Paths.get(path).normalize(); 
        
               final Path pathBase = Paths.get(baseDirectory).normalize();

romani · 2023-10-30T01:52:56Z

Github, generate report

github-actions · 2023-10-30T01:55:22Z

Report generation failed on phase "make_report",
step "Generate report".
Link: https://github.com/checkstyle/checkstyle/actions/runs/6687338433

https://github.com/checkstyle/checkstyle/actions/runs/6687338433/job/18167872017#step:9:578

Caused by: java.lang.IllegalArgumentException: 'other' is different type of Path
    at sun.nio.fs.UnixPath.relativize (UnixPath.java:429)
    at sun.nio.fs.UnixPath.relativize (UnixPath.java:43)
    at com.puppycrawl.tools.checkstyle.utils.CommonUtil.relativizeAndNormalizePath (CommonUtil.java:266)
    at com.puppycrawl.tools.checkstyle.Checker.acceptFileStarted (Checker.java:377)
    at com.puppycrawl.tools.checkstyle.Checker.processFiles (Checker.java:291)
    at com.puppycrawl.tools.checkstyle.Checker.process (Checker.java:226)

https://github.com/openjdk/jdk/blob/db3402577a2c14a41045753a1ffe2829a6bdda91/src/java.base/unix/classes/sun/nio/fs/UnixPath.java#L478-L495

    public UnixPath relativize(Path obj) {
        UnixPath child = toUnixPath(obj);
        if (child.equals(this))
            return emptyPath();

        // can only relativize paths of the same type
        if (this.isAbsolute() != child.isAbsolute())
            throw new IllegalArgumentException("'other' is different type of Path");

rdiachenko

lgtm

normalization is done as part of relativize for UnixPath

romani · 2023-10-31T12:30:19Z

regression report needs to pass, I am not sure why it is failing with exception.

Kevin222004 · 2023-11-05T19:17:22Z

Github, generate report

github-actions · 2023-11-05T19:19:26Z

Report generation failed on phase "make_report",
step "Generate report".
Link: https://github.com/checkstyle/checkstyle/actions/runs/6763272802

Vyom-Yadav · 2023-11-06T05:19:44Z

@romani @Kevin222004 I think there is a problem with relativizing path.

Caused by: java.lang.IllegalArgumentException: 'other' is different type of Path
    at sun.nio.fs.UnixPath.relativize (UnixPath.java:429)
    at sun.nio.fs.UnixPath.relativize (UnixPath.java:43)
    at com.puppycrawl.tools.checkstyle.utils.CommonUtil.relativizeAndNormalizePath (CommonUtil.java:266)

You may want to take a look at https://stackoverflow.com/q/16299604/15412365

romani · 2023-11-07T14:26:28Z

problem is at relativizeAndNormalizePath , so it is execution without update of this PR.
so exception is not related to PR update, and this is just bad config.

config is updated:

- <property name="basedir" value="."/>
+ <property name="basedir" value="/work/checkstyle/checkstyle/.ci-temp/contribution/checkstyle-tester/"/>

romani · 2023-11-07T14:30:27Z

Github, generate report

romani · 2023-11-08T02:37:05Z

Github, generate report

github-actions · 2023-11-08T03:50:56Z

Regression-with-path-for-basedir:
https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/8eeb6be_2023035007/reports/diff/index.html

romani · 2023-11-08T04:13:54Z

@rnveach , report is ready.

romani

Ok to merge

rnveach · 2023-11-08T18:46:43Z

@romani So we are not going to do anything on #13953 (comment) ?

romani · 2023-11-08T23:44:40Z

@rnveach , yes, let's have regression if it's actually matters, I will learn a lot from such usage :).
I don't know how to deal with such path in zip for checkstyle.

rnveach · 2023-11-14T14:34:44Z

Apparently this change failed in backport (JDK 8). Nothing changed in the javadoc for Path.relativize.

romani requested changes Oct 27, 2023

View reviewed changes

romani approved these changes Oct 29, 2023

View reviewed changes

romani requested a review from rdiachenko October 29, 2023 13:49

romani assigned rdiachenko Oct 29, 2023

romani requested a review from Vyom-Yadav October 29, 2023 13:49

rnveach requested a review from nrmancuso October 29, 2023 14:27

romani force-pushed the CommonUtil branch from 80cde19 to d9c038b Compare October 29, 2023 17:49

rdiachenko approved these changes Oct 30, 2023

View reviewed changes

Issue checkstyle#13809: Kill mutation in CommonUtil

8eeb6be

Kevin222004 force-pushed the CommonUtil branch from d9c038b to 8eeb6be Compare October 30, 2023 19:47

romani assigned Vyom-Yadav and unassigned rdiachenko Oct 30, 2023

romani self-requested a review October 31, 2023 12:29

romani assigned romani and unassigned Vyom-Yadav Oct 31, 2023

romani mentioned this pull request Oct 31, 2023

Issue #2827: JavadocMethod doesn't report anonymous class methods #13968

Closed

romani assigned rnveach Nov 8, 2023

romani approved these changes Nov 8, 2023

View reviewed changes

romani removed their assignment Nov 8, 2023

rnveach approved these changes Nov 12, 2023

View reviewed changes

rnveach assigned nrmancuso and unassigned rnveach Nov 12, 2023

nrmancuso approved these changes Nov 12, 2023

View reviewed changes

nrmancuso merged commit d52eb5d into checkstyle:master Nov 12, 2023
112 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #13809: Kill mutation in CommonUtil #13953

Issue #13809: Kill mutation in CommonUtil #13953

Kevin222004 commented Oct 26, 2023 •

edited by romani

romani left a comment

romani Oct 27, 2023

romani Oct 27, 2023

Kevin222004 Oct 29, 2023 •

edited

romani Oct 29, 2023

romani Oct 29, 2023 •

edited

rnveach Oct 30, 2023

romani Oct 30, 2023 •

edited

romani left a comment

rnveach commented Oct 29, 2023

romani commented Oct 29, 2023 •

edited

rnveach commented Oct 29, 2023

romani commented Oct 29, 2023

romani commented Oct 29, 2023

github-actions bot commented Oct 29, 2023

romani commented Oct 29, 2023

rnveach commented Oct 29, 2023 •

edited

romani commented Oct 30, 2023

github-actions bot commented Oct 30, 2023 •

edited by romani

rdiachenko left a comment

romani commented Oct 31, 2023

Kevin222004 commented Nov 5, 2023

github-actions bot commented Nov 5, 2023

Vyom-Yadav commented Nov 6, 2023

romani commented Nov 7, 2023 •

edited

romani commented Nov 7, 2023

romani commented Nov 8, 2023

github-actions bot commented Nov 8, 2023

romani commented Nov 8, 2023

romani left a comment

rnveach commented Nov 8, 2023

romani commented Nov 8, 2023 •

edited

rnveach commented Nov 14, 2023

	public void testRelativeNormalizedPathWithDenormalizedBaseDirectory() throws IOException {
	final String sampleAbsolutePath = new File("src/main/java").getCanonicalPath();
	final String absoluteFilePath = sampleAbsolutePath + "/SampleFile.java";
	final String basePath = sampleAbsolutePath + PATH_DENORMALIZER;

Issue #13809: Kill mutation in CommonUtil #13953

Issue #13809: Kill mutation in CommonUtil #13953

Conversation

Kevin222004 commented Oct 26, 2023 • edited by romani

romani left a comment

Choose a reason for hiding this comment

romani Oct 27, 2023

Choose a reason for hiding this comment

romani Oct 27, 2023

Choose a reason for hiding this comment

Kevin222004 Oct 29, 2023 • edited

Choose a reason for hiding this comment

romani Oct 29, 2023

Choose a reason for hiding this comment

romani Oct 29, 2023 • edited

Choose a reason for hiding this comment

rnveach Oct 30, 2023

Choose a reason for hiding this comment

romani Oct 30, 2023 • edited

Choose a reason for hiding this comment

romani left a comment

Choose a reason for hiding this comment

rnveach commented Oct 29, 2023

romani commented Oct 29, 2023 • edited

rnveach commented Oct 29, 2023

romani commented Oct 29, 2023

romani commented Oct 29, 2023

github-actions bot commented Oct 29, 2023

romani commented Oct 29, 2023

rnveach commented Oct 29, 2023 • edited

romani commented Oct 30, 2023

github-actions bot commented Oct 30, 2023 • edited by romani

rdiachenko left a comment

Choose a reason for hiding this comment

romani commented Oct 31, 2023

Kevin222004 commented Nov 5, 2023

github-actions bot commented Nov 5, 2023

Vyom-Yadav commented Nov 6, 2023

romani commented Nov 7, 2023 • edited

romani commented Nov 7, 2023

romani commented Nov 8, 2023

github-actions bot commented Nov 8, 2023

romani commented Nov 8, 2023

romani left a comment

Choose a reason for hiding this comment

rnveach commented Nov 8, 2023

romani commented Nov 8, 2023 • edited

rnveach commented Nov 14, 2023

Kevin222004 commented Oct 26, 2023 •

edited by romani

Kevin222004 Oct 29, 2023 •

edited

romani Oct 29, 2023 •

edited

romani Oct 30, 2023 •

edited

romani commented Oct 29, 2023 •

edited

rnveach commented Oct 29, 2023 •

edited

github-actions bot commented Oct 30, 2023 •

edited by romani

romani commented Nov 7, 2023 •

edited

romani commented Nov 8, 2023 •

edited