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

Allow for multiple method parameters to be @MustCallAlias in Resource Leak Checker; fixes #4785 #4808

Merged
merged 13 commits into from Jul 29, 2021

Conversation

Nargeshdb
Copy link
Contributor

Fixes #4785.

In this PR, methods and constructors are allowed to have Multiple @MustCallAlias arguments like:

public @MustCallAlias SequenceInputStream(@MustCallAlias InputStream s1, @MustCallAlias InputStream s2) {...}

If a call has @MustCallAlias annotation, then for each argument passed in the @MustCallAlias position, it creates a set containing the argument and it's resource aliases and add it to the set of obligations. Consider the following example:

SequenceInputStream s3 = new SequenceInputStream(s1, s2)

It creates two sets {s1, s3} and {s2, s3}, and then adds them to the set of obligations.

@mernst mernst changed the title Allow for multiple method parameters to be @MustCallAlias in Resource Leak Checker #4785 Allow for multiple method parameters to be @MustCallAlias in Resource Leak Checker; fixes #4785 Jul 19, 2021
Copy link
Contributor

@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.

Mostly looks good! I have a few comments

@mernst mernst assigned kelloggm and unassigned msridhar Jul 26, 2021
@kelloggm kelloggm assigned msridhar and unassigned kelloggm Jul 26, 2021
Copy link
Contributor

@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 mostly good to me; just one more comment. Please re-assign to me once it is addressed.

@msridhar msridhar assigned Nargeshdb and unassigned msridhar Jul 27, 2021
Copy link
Contributor

@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.

Just one more comment

Copy link
Contributor

@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.

Went ahead and addressed my final comment. Now looks good to go

@msridhar msridhar assigned kelloggm and unassigned Nargeshdb Jul 27, 2021
@msridhar msridhar requested a review from kelloggm July 27, 2021 15:35
Copy link
Contributor

@kelloggm kelloggm left a comment

Choose a reason for hiding this comment

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

Overall, this looks good to me, modulo a few relatively minor comments.

.allMatch(
mustCallAliasArgument ->
mustCallAliasArgument instanceof FieldAccessNode
|| mustCallAliasArgument instanceof ThisNode);
Copy link
Contributor

Choose a reason for hiding this comment

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

In the Checker Framework, the preferred style is to use a loop rather than a stream. Can you rewrite this to use a loop rather than a stream?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@msridhar msridhar requested a review from kelloggm July 29, 2021 00:36
@msridhar
Copy link
Contributor

@kelloggm do you have any further comments on this one?

@kelloggm kelloggm merged commit 52182ca into typetools:master Jul 29, 2021
wmdietl pushed a commit to eisop/checker-framework that referenced this pull request Aug 3, 2021
wmdietl pushed a commit to eisop/checker-framework that referenced this pull request Aug 3, 2021
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.

Allow for multiple method parameters to be @MustCallAlias in Resource Leak Checker
3 participants