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

Fix empty contract arguments #616

Merged
merged 6 commits into from Jul 4, 2022
Merged

Fix empty contract arguments #616

merged 6 commits into from Jul 4, 2022

Conversation

Sparky983
Copy link
Contributor

@Sparky983 Sparky983 commented Jun 22, 2022

Fixed a bug where empty contract arguments would return a singleton array of the contract arguments (empty).
before:

ContractUtils.getAntecedent("    -> hi", ...);
// {"    "}

after:

ContractUtils.getAntecedent("    -> hi", ...);
// {}

Thank you for contributing to NullAway!

Please note that once you click "Create Pull Request" you will be asked to sign our Uber Contributor License Agreement via CLA assistant.

Before pressing the "Create Pull Request" button, please provide the following:

  • A description about what and why you are contributing, even if it's trivial.

  • The issue number(s) or PR number(s) in the description if you are contributing in response to those.

  • If applicable, unit tests.

Fix bug where empty contract arguments would return a singleton array of the contract arguments (empty).
@CLAassistant
Copy link

CLAassistant commented Jun 22, 2022

CLA assistant check
All committers have signed the CLA.

@msridhar
Copy link
Collaborator

Thanks for the contribution! I see how this fixes a bug in parsing of @Contract annotations. I'm curious, though, is there a scenario in which this bug would have impacted NullAway's results? If so, could we write a test that shows the impact of the fix?

@Sparky983
Copy link
Contributor Author

This shouldn't be the case because it checks if the first part of the contract clause (before the "->") is blank which is only possible when no args are present.

Replaced isEmpty() (11) call with trim().isBlank() (1.8)
@@ -68,6 +68,25 @@ public void basicContractAnnotation() {
.doTest();
}

@Test
public void contractArgsEmpty() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than this test, would it be possible to write a separate test that directly calls into the APIs of ContractUtils and asserts that the appropriate string is parsed as intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing

Copy link
Collaborator

@msridhar msridhar left a comment

Choose a reason for hiding this comment

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

Almost looks good to me! One more comment

public void getAntecedentCorrectNumOfArgs() {
ContractUtils.getAntecedent("->_", tree, analysis, state, symbol, 0);

verifyNoInteractions(tree, state, analysis, symbol);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we merge this call into the previous test, as the last line? I don't see why we need a separate test for this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

Copy link
Contributor Author

@Sparky983 Sparky983 Jul 4, 2022

Choose a reason for hiding this comment

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

I'm not quite sure why I even separated them🤔

Copy link
Collaborator

@msridhar msridhar left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@msridhar msridhar enabled auto-merge (squash) July 4, 2022 03:18
@msridhar msridhar merged commit 4fc0693 into uber:master Jul 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants