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 UnusedVariable false positives for private record parameters #3837

Closed
wants to merge 4 commits into from
Closed

Fix UnusedVariable false positives for private record parameters #3837

wants to merge 4 commits into from

Conversation

Marcono1234
Copy link
Contributor

@Marcono1234 Marcono1234 commented Mar 27, 2023

Fixes #2713

It looks like the tree emitted by the compiler is incomplete for the canonical constructor of a record, which is why the parameters were erroneously flagged as unused. Apparently this only affected private record classes because for them the implicit canonical constructor is private as well, and the UnusedVariable check treats parameters of private methods and constructors differently.

#2713 (comment) showed an example where this also affected record fields, however I was unable to reproduce that. Maybe they were using an older Error Prone version which does not include 47da3af, their sample code is incomplete, or my test setup was incorrect.
Edit: It looks like they were using an older Error Prone version, see #2713 (comment).

@Marcono1234
Copy link
Contributor Author

@cushon, are the changes ok like this or do you want anything changed?

Copy link
Collaborator

@cushon cushon left a comment

Choose a reason for hiding this comment

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

Thanks for the fix, and sorry for the slow review!

// to contain usage in that case, but parameter is always used implicitly
// For compact canonical constructor parameters don't have record flag so need to
// check constructor flags (`symbol.owner`) instead
if (hasRecordFlag(symbol) || hasRecordFlag(symbol.owner)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are there any cases where hasRecordFlag(symbol) is true but hasRecordFlag(symbol.owner) isn't?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you asking if this could be simplified to only check hasRecordFlag(symbol.owner)?
It looks like the tests still pass if I locally remove the additional hasRecordFlag(symbol) check, should I change this then?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you asking if this could be simplified to only check hasRecordFlag(symbol.owner)?

Yes

It looks like the tests still pass if I locally remove the additional hasRecordFlag(symbol) check, should I change this then?

I think so, unless you suspect there might be cases where checking both is necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unless you suspect there might be cases where checking both is necessary?

I am not aware of any; have changed it now to only check symbol.owner.

copybara-service bot pushed a commit that referenced this pull request Aug 15, 2023
Fixes #2713

It looks like the tree emitted by the compiler is incomplete for the canonical constructor of a record, which is why the parameters were erroneously flagged as unused. Apparently this only affected `private` record classes because for them the implicit canonical constructor is `private` as well, and the `UnusedVariable` check treats parameters of `private` methods and constructors differently.

#2713 (comment) showed an example where this also affected record fields, however I was unable to reproduce that. Maybe they were using an older Error Prone version which does not include 47da3af, their sample code is incomplete, or my test setup was incorrect.
Edit: It looks like they were using an older Error Prone version, see #2713 (comment).

Fixes #3837

FUTURE_COPYBARA_INTEGRATE_REVIEW=#3837 from Marcono1234:record-unused-param-false-positives 4535778
PiperOrigin-RevId: 557194050
copybara-service bot pushed a commit that referenced this pull request Aug 17, 2023
Fixes #2713

It looks like the tree emitted by the compiler is incomplete for the canonical constructor of a record, which is why the parameters were erroneously flagged as unused. Apparently this only affected `private` record classes because for them the implicit canonical constructor is `private` as well, and the `UnusedVariable` check treats parameters of `private` methods and constructors differently.

#2713 (comment) showed an example where this also affected record fields, however I was unable to reproduce that. Maybe they were using an older Error Prone version which does not include 47da3af, their sample code is incomplete, or my test setup was incorrect.
Edit: It looks like they were using an older Error Prone version, see #2713 (comment).

Fixes #3837

FUTURE_COPYBARA_INTEGRATE_REVIEW=#3837 from Marcono1234:record-unused-param-false-positives 4535778
PiperOrigin-RevId: 557197001
copybara-service bot pushed a commit that referenced this pull request Aug 17, 2023
Fixes #2713

It looks like the tree emitted by the compiler is incomplete for the canonical constructor of a record, which is why the parameters were erroneously flagged as unused. Apparently this only affected `private` record classes because for them the implicit canonical constructor is `private` as well, and the `UnusedVariable` check treats parameters of `private` methods and constructors differently.

#2713 (comment) showed an example where this also affected record fields, however I was unable to reproduce that. Maybe they were using an older Error Prone version which does not include 47da3af, their sample code is incomplete, or my test setup was incorrect.
Edit: It looks like they were using an older Error Prone version, see #2713 (comment).

Fixes #3837

FUTURE_COPYBARA_INTEGRATE_REVIEW=#3837 from Marcono1234:record-unused-param-false-positives 4535778
PiperOrigin-RevId: 557197001
@Marcono1234 Marcono1234 deleted the record-unused-param-false-positives branch August 17, 2023 20:45
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.

UnusedVariable sometimes gives false positives for record parameters
2 participants