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

New check: ConstructorWithoutParamsCheck to catch Throwable constructors usage with no arguments #412

Closed
sdudoladov opened this issue Dec 21, 2015 · 18 comments
Milestone

Comments

@sdudoladov
Copy link
Contributor

@sdudoladov sdudoladov commented Dec 21, 2015

An exception message must contain enough info to find out why an exception occurred (e.g. "Effective Java", item 63). So a check that forbids

  • using Throwable no-args constructors
  • passing empty String literals to Throwable constructors.

may be worth considering.

@sdudoladov sdudoladov changed the title New check: New check: forbid non-informative Throwable constructors Dec 21, 2015
@romani
Copy link
Member

@romani romani commented Dec 21, 2015

first request should be done as general new Check - NoDefaultCtorUsage or smth similar
second request should be as EmptyExceptionMessage, user define regexp of how to distinguish classes from Exception/Error/Throwable

@sdudoladov
Copy link
Contributor Author

@sdudoladov sdudoladov commented Jan 25, 2016

@romani I consider writing the NoDefaultCtorUsage check. Is anyone already working on it ?

second request should be as EmptyExceptionMessage, user define regexp

This is the project-specific RegExp and not a new check, right ?

@romani
Copy link
Member

@romani romani commented Jan 27, 2016

@zerg-junior ,

I consider writing the NoDefaultCtorUsage check. Is anyone already working on it ?

I try to keep assignments of issue (by comment or Assigneed field) if somebody what to implement issue.
Nobody is focused on this.
We do not have a constantly focused engineers on the project, you are always welcome to help with implementations

This is the project-specific RegExp and not a new check, right ?

customized RegExp Check will not help to cover case reliably without false positives.
Special new Check will be more reliable but still not 100% working solution. Checkstyle have limitation and do not have information about Types and their hierarchy - we just have tree of strings.
http://checkstyle.sourceforge.net/writingchecks.html#Limitations
So wee need to help Check to catch Exception types by RegExp like "._Exception|._Error|.*Throwable" , if we find code like "new MyException("");" -- you raise an violation.
I could provide you more details and guide you through implementation, just let me know if you with to continue.

@sdudoladov
Copy link
Contributor Author

@sdudoladov sdudoladov commented Jan 27, 2016

you are always welcome to help with implementations

Great, then please skim the following contract:

NoCtorWithoutParamsCheck

Prohibits usage of parameterless constructors, including the default ones.

Rationale: constructors of certain classes must always take arguments to properly instantiate objects. Exception classes are the primary example: their objects must always contain enough info to find out why an exception occurred (see "Effective Java", item 63). Constructing an exception without a cause exception or an exception message leaves out such info and thus should be prohibited.

Configuration: via the format field of the RegExp type:

<module name="NoCtorWithoutParamsCheck">
  <property name="format" value=".*Exception|Clazz1"/>
</module>
Examples
// the check can prohibit default constructors of built-in classes
RuntimeException ex = new RuntimeException(); // prohibited

// the check can prohibit default constructors of user-defined classes
public class Clazz1 {

}

Clazz1 o1 = new Clazz1(); // prohibited

// the check can prohibit  user-defined parameterless constructors 
public class Clazz1 {

  Clazz1() {
    foobar();
  }

}

Clazz1 o1 = new Clazz1(); // prohibited
Non-examples
// here the cosntructor takes a String argument, albeit an empty one
RuntimeException ex = new RuntimeException(""); //allowed

Does it make sense ? I leave handling empty String literals to a separate check.

@romani
Copy link
Member

@romani romani commented Jan 28, 2016

NoCtorWithoutParamsCheck

make a name ConstructorWithoutParamsCheck , name of the Check helps to find it easily. "No" is not required as Check just catch them , how to fix code is not a Check business.

format

name of option and meaning of it should be clear without reading documentation. I propose "typeNameFormat".

Non-examples

I do not understand why this is not a example. All of them are examples that show user where violation will be and what cases we skip. User like to read examples rather then descriptions.

Does it make sense ?

Just to let you consider one nuance. If we go by matching of class type by regexp it could mean that some false positives could appear if the same name (but different package) is used. We could extend a bit Check to ask user to define canonical name of type (com.mycompany.MyClass) and analyse imports to make sure that we matching "new MyClass()" of com.mycompany.MyClass and not matching com.yourcompany.MyClass. We already have such Checks , implementation is not a poblem. The only drawback is that it demands from user detailed configuration with full names. ".*Exception" is much easier :) . Both approaches a valid.
What are your thoughts about this ? What way you will go ?

I leave handling empty String literals to a separate check.

please create separate issue for this.

@sdudoladov
Copy link
Contributor Author

@sdudoladov sdudoladov commented Jan 29, 2016

The only drawback is that it demands from user detailed configuration with full names. ".*Exception" is much easier :) . Both approaches a valid.
What are your thoughts about this ? What way you will go ?

I actually need this check for exceptions, so I'll go with the reg exp.

@romani romani changed the title New check: forbid non-informative Throwable constructors New check: ConstructorWithoutParamsCheck to catch Throwable constructors usage with no arguments Jan 29, 2016
@romani
Copy link
Member

@romani romani commented Jan 29, 2016

you are welcome.
issue is renamed to be ConstructorWithoutParamsCheck specific.

@sdudoladov
Copy link
Contributor Author

@sdudoladov sdudoladov commented Feb 3, 2016

@romani I suggest we leave the default config empty for this check. Is it reasonable ?

@romani
Copy link
Member

@romani romani commented Feb 3, 2016

I think it ok to make it ".*Exception" by default. Createion of exceprion without explanation is evil.

@sdudoladov
Copy link
Contributor Author

@sdudoladov sdudoladov commented Feb 3, 2016

@romani Seems plausible, I'll do it.

@romani
Copy link
Member

@romani romani commented Feb 5, 2016

let me know if you need help with implementation or IDE setup or ..... .
We have bunch of examples in code :) on how to make a Check.

@sdudoladov
Copy link
Contributor Author

@sdudoladov sdudoladov commented Feb 11, 2016

let me know if you need help with implementation or IDE setup or

In BaseCheckTestSupport, the BriefLogger calls the super() of DefaultLogger with the fifth parameter being boolean. The only 5-parameter DefaultLogger constructor seems to expect an AuditEventFormatter value in this place. Hence compilation failure due to incompatible types.

Am I using an outdated version of the core checkstyle or miss some other library?

@romani
Copy link
Member

@romani romani commented Feb 11, 2016

Please look at our build system https://travis-ci.org/sevntu-checkstyle/sevntu.checkstyle it is green so it is compilable. Compare compilation process with it. If all the same please share with full log of compilation at gist.github.com

@sdudoladov
Copy link
Contributor Author

@sdudoladov sdudoladov commented Feb 16, 2016

@romani Does the following log message look meaningful to you ?

Calls to constructors of ''{0}'' should use at least one parameter.

''{0}'' stands for the class name.

Also, does sevntu-checkstyle have any special contributor guidelines ?

sdudoladov pushed a commit to sdudoladov/sevntu.checkstyle that referenced this issue Feb 16, 2016
This patch adds the ConstructorWithoutParamsCheck

GitHub:
 [sevntu.checkstyle issue 412] (sevntu-checkstyle#412)
sdudoladov pushed a commit to sdudoladov/sevntu.checkstyle that referenced this issue Feb 16, 2016
This patch adds the ConstructorWithoutParamsCheck

GitHub:
 [sevntu.checkstyle issue 412] (sevntu-checkstyle#412)
@sdudoladov
Copy link
Contributor Author

@sdudoladov sdudoladov commented Feb 16, 2016

@romani
Copy link
Member

@romani romani commented Feb 16, 2016

Does the following log message look meaningful to you ?

Yes, looks clear.

does sevntu-checkstyle have any special contributor guidelines ?

Please rephrase a question, I do not understand it.

sdudoladov pushed a commit to sdudoladov/sevntu.checkstyle that referenced this issue Feb 17, 2016
This patch adds the ConstructorWithoutParamsCheck

GitHub:
 [sevntu.checkstyle issue 412] (sevntu-checkstyle#412)
sdudoladov pushed a commit to sdudoladov/sevntu.checkstyle that referenced this issue Mar 1, 2016
sdudoladov pushed a commit to sdudoladov/sevntu.checkstyle that referenced this issue Mar 1, 2016
sdudoladov pushed a commit to sdudoladov/sevntu.checkstyle that referenced this issue Mar 9, 2016
sdudoladov pushed a commit to sdudoladov/sevntu.checkstyle that referenced this issue Mar 9, 2016
sdudoladov pushed a commit to sdudoladov/sevntu.checkstyle that referenced this issue Mar 14, 2016
sdudoladov pushed a commit to sdudoladov/sevntu.checkstyle that referenced this issue Mar 14, 2016
sdudoladov pushed a commit to sdudoladov/sevntu.checkstyle that referenced this issue Mar 14, 2016
sdudoladov pushed a commit to sdudoladov/sevntu.checkstyle that referenced this issue Mar 18, 2016
sdudoladov pushed a commit to sdudoladov/sevntu.checkstyle that referenced this issue Mar 18, 2016
sdudoladov pushed a commit to sdudoladov/sevntu.checkstyle that referenced this issue Mar 18, 2016
romani added a commit that referenced this issue Mar 21, 2016
@romani romani added this to the 1.20 milestone Mar 21, 2016
@romani
Copy link
Member

@romani romani commented Mar 21, 2016

fix is merged.

@romani romani closed this Mar 21, 2016
@romani
Copy link
Member

@romani romani commented Mar 25, 2016

@zerg-junior , your Check is released, please enforce it over checkstyle code - checkstyle/checkstyle#3064

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

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.