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

Issue #13772: IllegalImport with class pattern fails on static import #13960

Closed

Conversation

AayushSaini101
Copy link
Contributor

Related: #13772

@@ -139,7 +139,7 @@ public final void setIllegalPkgs(String... from) {
illegalPkgs = from.clone();
illegalPkgsRegexps.clear();
for (String illegalPkg : illegalPkgs) {
illegalPkgsRegexps.add(CommonUtil.createPattern("^" + illegalPkg + "\\..*"));
illegalPkgsRegexps.add(CommonUtil.createPattern(illegalPkg + "\\..*"));
Copy link
Member

@rnveach rnveach Oct 31, 2023

Choose a reason for hiding this comment

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

Won't this possibly create false matches like if my illegal package is blah and my input file has a package com.other.blah.other (or similar)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rnveach This will cause an error when the blah is found in the package, and this is an illegal package

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure I follow what you mean by an error. I assume violation.

But my question was would this be a false match. If I say my illegal package is blah, I expect to only match blah or blah.other depending on the rest of the criteria for this module. I wouldn't expect a match on com.other.blah.other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure I follow what you mean by an error. I assume violation.

But my question was would this be a false match. If I say my illegal package is blah, I expect to only match blah or blah.other depending on the rest of the criteria for this module. I wouldn't expect a match on com.other.blah.other.

Thanks @rnveach In this case it will give false violation i removed the ^ symbol, can we add some conditions like only static or some more keywords allow only

Copy link
Member

@rnveach rnveach Oct 31, 2023

Choose a reason for hiding this comment

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

You would need to examine the module closer to see if there is some conditional statements that would have to be added. Like other issues, I have not looked into the full details of the issue and code.

However, to me it does seem odd that this issue can be resolved with what amounts to a configuration change from looking at this PR. If your changing the code in the property setter, your basically saying either how we read the value from the configuration, or the contents of the configuration, are wrong (like "" should be seen as null instead of an empty string or such).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@romani @rnveach There are the proper usage how to use import in different, For this issue, We need a condition only for handling the static instead of removing the "^" from the import check.

  • Single Type Import:

This is the most common usage of the import statement and is used to import a single class or interface.

import java.util.ArrayList;
Import All Classes from a Package:
You can import all classes and interfaces from a specific package by using the * wildcard.

import java.util.*;
This will import all classes and interfaces within the java.util package.

  • Static Import:

The import statement can also be used to import static members (fields and methods) of a class. This is often used for constants or utility methods.

import static java.lang.Math.*;
This allows you to use static members of the Math class directly without prefixing with Math..

  • On-Demand Type Import:

This is similar to importing all classes from a package but only imports the specific class you use in your code.

import java.util.ArrayList;
import java.util.HashMap;

In this example, only ArrayList and HashMap are imported from the java.util package, even though other classes and interfaces are available in that package.

  • Import for Nested Classes:

If you have a nested class, you can import it in a similar way, using the nested class name.

import mypackage.OuterClass.InnerClass;
This imports the InnerClass which is a nested class within OuterClass.

Copy link
Member

@rnveach rnveach Nov 2, 2023

Choose a reason for hiding this comment

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

java.util.HashMap
mypackage.OuterClass.InnerClass

Just pointing out that you can't identify an inner class from a regular class without multi-file and/or classpath support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rnveach @romani can we add separate property for this for handling static import?

Copy link
Member

Choose a reason for hiding this comment

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

It is completely impossible for checkstyle to cover it, so we simply need to skip such cases from validation.

@romani
Copy link
Member

romani commented Nov 1, 2023

While you try to do code analysis, you can create diff report by GitHub action and it might catch regression for you, so you will have ready test input file where regression is visible.

@aayushRedHat
Copy link
Contributor

@romani @rnveach What update i need to do this in issue ?

@rnveach
Copy link
Member

rnveach commented Nov 17, 2023

@aayushRedHat See #13960 (comment)

@romani
Copy link
Member

romani commented Nov 17, 2023

@aayushRedHat, please create new PR with account aayushRedHat, to have in CI all 113 executions

@aayushRedHat
Copy link
Contributor

@aayushRedHat, please create new PR with account aayushRedHat, to have in CI all 113 executions

Sure @romani Closing This PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants