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

MethodName check fails on constructor of nested class #1557

Closed
rasmusbonnedal opened this issue Sep 21, 2018 · 4 comments · Fixed by #1559
Closed

MethodName check fails on constructor of nested class #1557

rasmusbonnedal opened this issue Sep 21, 2018 · 4 comments · Fixed by #1559
Assignees
Labels
Milestone

Comments

@rasmusbonnedal
Copy link

Description

MethodName check misidentifies the constructor of a nested class as a method

Steps to reproduce the problem

Add this to cxx-checks/src/test/resources/checks/MethodName.cc and run mvn test

class OuterClass {
  class Inner_Class {
    Inner_Class();
  };
};

OuterClass::Inner_Class::Inner_Class()
{
}

Expected behavior

That Inner_Class constructor would not fail MethodName check.

Actual behavior

Inner_Class constructor fails MethodName check.

Known workarounds

Please provide a description of any known workarounds.

LOG file

Output from mvn test

[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.018 s <<< FAILURE! - in org.sonar.cxx.checks.naming.MethodNameCheckTest
[ERROR] test(org.sonar.cxx.checks.naming.MethodNameCheckTest)  Time elapsed: 0.018 s  <<< FAILURE!
java.lang.AssertionError: 

No more violations expected
got: at line 51
        at org.sonar.cxx.checks.naming.MethodNameCheckTest.test(MethodNameCheckTest.java:50)

Related information

  • cxx plugin version: latest master 2018-09-21 (g071070...). Same result in 1.1.0
  • SonarQube version: 6.7.5
@ivangalkin
Copy link
Contributor

ivangalkin commented Sep 21, 2018

Hello @rasmusbonnedal and thank you for reaching out.

Do you use some custom regexp to ensure the naming convention?

If not: there is probably no relation to the nesting. As you see...

public class MethodNameCheck extends SquidCheck<Grammar> {

  private static final String DEFAULT = "^[A-Z][A-Za-z0-9]{2,30}$";

... the underscore is considered to be an undesirable part of methods' names by default (see CamelCase). In case you would like to accept it, please navigate to the rule cxx.MethodName/c.MethodName and customize the format according to your naming convention (in case format is read-only, you should create a copy of default quality profile and use this copied profile in your project).

Regards,

@rasmusbonnedal
Copy link
Author

Hi @ivangalkin, thanks for your quick response!

We do have a custom regexp but I still think this is a general issue and indeed related to nesting.

In MethodNameCheck.java there is code to detect if the method is a constructor and in that case not enforce the naming convention. This allows us to have class name convention != method name convention and not get issues on all constructors. This works well in outer classes but fails in inner classes. To illustrate this I changed my test case in MethodName.cc to this:

class Outer_Class {
  Outer_Class();
  class Inner_Class {
    Inner_Class();
  };
};

Outer_Class::Outer_Class() // This does not give an error
{
}

OuterClass::Inner_Class::Inner_Class() // This gives an error
{
}

I think this discrepancy between outer and inner class constructors in MethodName check is not expected.

@ivangalkin
Copy link
Contributor

Hi @rasmusbonnedal,

I double-checked the code. MethodNameCheck.java applies the regexp only if the method is considered to be not a constructor and not a destructor (by the way I believe the destructor detection might be incomplete). So you're right.

Inner_Class::Inner_Class() is not recognized as a constructor implementation. Thereby the constructor declaration works well (at least in your example).

In any way I confirm the bug. We have to modify the constructor/destructor detection.

Thank you

@rasmusbonnedal
Copy link
Author

@ivangalkin: Awesome, thanks!

@guwirth guwirth added the bug label Sep 21, 2018
@guwirth guwirth added this to the 1.1 milestone Sep 21, 2018
ivangalkin added a commit to ivangalkin/sonar-cxx that referenced this issue Sep 23, 2018
* Modify the grammar. Specify the nestedNameSpecifier in terms of `className`s
  * the naming check is based on extraction of `className`s
  * grammar of `className` is equivalent to `typeName` and covers both `simpleTemplateId` and `IDENTIFIER`

```JAVA
    b.rule(typeName).is(
      b.firstOf(
        className,       // == b.firstOf( simpleTemplateId, IDENTIFIER )
        enumName,        // == IDENTIFIER
        typedefName,     // == IDENTIFIER
        simpleTemplateId //
      )
    );
```

* Modify MethodNameCheck: the scope of the method/ctor/dtor is always
  represented by the most significant child of type `className`

```
  functionDefinition ==
     [namespaces...]::className0::className1::className2::ID
                                              ^^^^^^^^^^
```

closes SonarOpenCommunity#1557
ivangalkin added a commit to ivangalkin/sonar-cxx that referenced this issue Oct 5, 2018
@guwirth guwirth modified the milestones: 1.1, 1.2 Oct 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

3 participants