Skip to content

SONARPY-525 Rule S5655: Arguments given to functions should be of an expected type#672

Merged
guillaume-dequenne merged 3 commits intomasterfrom
SONARPY-525
Apr 15, 2020
Merged

SONARPY-525 Rule S5655: Arguments given to functions should be of an expected type#672
guillaume-dequenne merged 3 commits intomasterfrom
SONARPY-525

Conversation

@guillaume-dequenne
Copy link
Copy Markdown
Contributor

@guillaume-dequenne guillaume-dequenne commented Apr 8, 2020

No description provided.

@guillaume-dequenne guillaume-dequenne force-pushed the SONARPY-525 branch 16 times, most recently from d08f9b6 to 365c7f1 Compare April 9, 2020 14:10
andrea-guarino-sonarsource
andrea-guarino-sonarsource reviewed Apr 9, 2020
View reviewed changes
Comment thread python-frontend/src/main/java/org/sonar/python/semantic/FunctionSymbolImpl.java Outdated
Copy link
Copy Markdown
Contributor

@andrea-guarino-sonarsource andrea-guarino-sonarsource Apr 9, 2020

Choose a reason for hiding this comment

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

I have the feeling we shouldn't have this logic happening in FunctionSymbol.
Having an implicit parameter doesn't depend on how the function is defined, but instead on how /where the function is called.
The information we may miss today in FunctionSymbol interface is the names of the decorators

Copy link
Copy Markdown
Contributor Author

@guillaume-dequenne guillaume-dequenne Apr 14, 2020

Choose a reason for hiding this comment

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

I have removed that API and added a new one to retrieve decorators in d9e48824b4d75f149e2db5cfcac09b46ad1a8fe9.

Comment thread python-frontend/src/main/java/org/sonar/python/types/TypeShed.java Outdated
Copy link
Copy Markdown
Contributor

@andrea-guarino-sonarsource andrea-guarino-sonarsource Apr 9, 2020

Choose a reason for hiding this comment

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

I think I would prefer having a method setParametersDeclaredType(ParameterList parameterList) instead of having separate methods to clear and add

Copy link
Copy Markdown
Contributor Author

@guillaume-dequenne guillaume-dequenne Apr 14, 2020

Choose a reason for hiding this comment

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

Indeed! I refactored that part in d9e48824b4d75f149e2db5cfcac09b46ad1a8fe9.

Comment thread python-frontend/src/main/java/org/sonar/python/types/TypeShed.java Outdated
Comment on lines 194 to 209
Copy link
Copy Markdown
Contributor

@andrea-guarino-sonarsource andrea-guarino-sonarsource Apr 9, 2020

Choose a reason for hiding this comment

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

You can access it via FunctionDefImpl.functionSymbol()

Comment thread python-frontend/src/main/java/org/sonar/plugins/python/api/types/InferredType.java Outdated
Copy link
Copy Markdown
Contributor

@andrea-guarino-sonarsource andrea-guarino-sonarsource Apr 9, 2020

Choose a reason for hiding this comment

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

I don't like this name, what about canBeOrExtend(InferredType other)?

Comment thread python-frontend/src/main/java/org/sonar/python/semantic/SymbolTableBuilder.java Outdated
Copy link
Copy Markdown
Contributor

@andrea-guarino-sonarsource andrea-guarino-sonarsource Apr 9, 2020

Choose a reason for hiding this comment

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

Why can't we do it inside FunctionSymbolImpl constructor anymore?

Copy link
Copy Markdown
Contributor Author

@guillaume-dequenne guillaume-dequenne Apr 14, 2020

Choose a reason for hiding this comment

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

I tried adding that logic again in FunctionSymbolImpl constructor but encountered a different issue where some class symbols could have their super class read through type annotations before their type hierarchy was defined.
Therefore, in d9e48824b4d75f149e2db5cfcac09b46ad1a8fe9, I moved this logic to ThirdPhaseVisitor.

@guillaume-dequenne guillaume-dequenne force-pushed the SONARPY-525 branch 11 times, most recently from e58162a to d9e4882 Compare April 14, 2020 15:34
@guillaume-dequenne guillaume-dequenne marked this pull request as ready for review April 14, 2020 15:50
andrea-guarino-sonarsource
andrea-guarino-sonarsource suggested changes Apr 15, 2020
View reviewed changes
Comment thread python-frontend/src/main/java/org/sonar/python/semantic/FunctionSymbolImpl.java Outdated
Copy link
Copy Markdown
Contributor

@andrea-guarino-sonarsource andrea-guarino-sonarsource Apr 15, 2020

Choose a reason for hiding this comment

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

why declaredType is Nullable?

Copy link
Copy Markdown
Contributor Author

@guillaume-dequenne guillaume-dequenne Apr 15, 2020

Choose a reason for hiding this comment

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

I forgot to remove that...Fixed it in 6a53bddec8101c389dc573bcbf56eb2f79c5a585

Comment thread python-frontend/src/main/java/org/sonar/python/semantic/SymbolTableBuilder.java Outdated
Copy link
Copy Markdown
Contributor

@andrea-guarino-sonarsource andrea-guarino-sonarsource Apr 15, 2020

Choose a reason for hiding this comment

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

I think functionSymbol.is(Symbol.Kind.FUNCTION) shouldn't be necessary

Comment thread python-frontend/src/main/java/org/sonar/python/types/RuntimeType.java Outdated
Copy link
Copy Markdown
Contributor

@andrea-guarino-sonarsource andrea-guarino-sonarsource Apr 15, 2020

Choose a reason for hiding this comment

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

I don't understand why if otherFullyQualifiedName is null we call typeClass.hasUnresolvedTypeHierarchy()? Probably it's better to return true

Copy link
Copy Markdown
Contributor Author

@guillaume-dequenne guillaume-dequenne Apr 15, 2020

Choose a reason for hiding this comment

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

Indeed, also this case should normally never happen.
I fixed this in 6a53bddec8101c389dc573bcbf56eb2f79c5a585.
Please note that b275a488d001987df20f088dc8a1711c6ffe47fe also introduce changes to this method to account for duck type compatibility.

Comment thread python-frontend/src/test/java/org/sonar/python/types/RuntimeTypeTest.java Outdated
Copy link
Copy Markdown
Contributor

@andrea-guarino-sonarsource andrea-guarino-sonarsource Apr 15, 2020

Choose a reason for hiding this comment

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

I think it would be nice to have a test with a null fully qualified name

Comment thread python-checks/src/main/java/org/sonar/python/checks/ArgumentTypeCheck.java Outdated
Copy link
Copy Markdown
Contributor

@andrea-guarino-sonarsource andrea-guarino-sonarsource Apr 15, 2020

Choose a reason for hiding this comment

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

Maybe we can add a comment to explain why we ignore this case

Comment thread python-checks/src/test/resources/checks/argumentType.py Outdated
Copy link
Copy Markdown
Contributor

@andrea-guarino-sonarsource andrea-guarino-sonarsource Apr 15, 2020

Choose a reason for hiding this comment

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

I think we should have a test for functions defined with variadic parameters (*args and **kwargs)

Copy link
Copy Markdown
Contributor Author

@guillaume-dequenne guillaume-dequenne Apr 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Indeed! Actually that made me realize we don't handle them very well as we only rely on type annotation without knowing whether the parameter is variadic or not. In 6a53bddec8101c389dc573bcbf56eb2f79c5a585 I have therefore disabled the rule when a function has variadic parameters.

@guillaume-dequenne guillaume-dequenne force-pushed the SONARPY-525 branch 3 times, most recently from fe6de77 to c31e73a Compare April 15, 2020 12:18
andrea-guarino-sonarsource
andrea-guarino-sonarsource approved these changes Apr 15, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@andrea-guarino-sonarsource andrea-guarino-sonarsource left a comment

Choose a reason for hiding this comment

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

LGTM, as discussed before merging please split commits adding API from the rule implementation.

@sonarsource-next
Copy link
Copy Markdown

sonarsource-next Bot commented Apr 15, 2020

Kudos, SonarQube Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

98.9% 98.9% Coverage
0.0% 0.0% Duplication

@guillaume-dequenne guillaume-dequenne merged commit 9dea0b7 into master Apr 15, 2020
@guillaume-dequenne guillaume-dequenne deleted the SONARPY-525 branch April 15, 2020 14:15
hashicorp-vault-sonar-prod Bot pushed a commit that referenced this pull request Feb 2, 2026
@renovate
Update all non-major dependencies (#672)
8e230a6 
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
GitOrigin-RevId: 7a013b5b756bfc434de92ca7736265ca81de62ec
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@andrea-guarino-sonarsource andrea-guarino-sonarsource andrea-guarino-sonarsource approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@guillaume-dequenne @andrea-guarino-sonarsource