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

Return the "real" Symbol instead of sometimes a Symbol whose owner is a subclass. #4273

Closed
wants to merge 0 commits into from

Conversation

copybara-service[bot]
Copy link
Contributor

Return the "real" Symbol instead of sometimes a Symbol whose owner is a subclass.

We accomplish this by calling baseSymbol() on the initial Symbol.

(We likely should do this in more places in Error Prone, including the location in ASTHelpers.getSymbol that I've flagged and perhaps anywhere else that we directly access tree.sym.)

Thanks to @msridhar for links to relevant JDK discussions, notably JDK-8293890.

Effects:

  • This change definitely helps with static imports, as demonstrated in a new test in ParameterNameTest, which fails before this change. (I didn't actually look into why the test fails before. Maybe the "fake" Symbol lacks the parameter names of the original?)
  • I didn't check whether it helps with someSerializable.equals. It seems likely to, but shrug for now.
  • I had expected this to help with the TestCase.fail handling in ReturnMissingNullable, but it turns out that I'm wrong on multiple levels. I updated its comments and tests accordingly:
    • TestCase.fail does exist as a declared method nowadays; it's not merely inherited from Assert. I must have been looking at quite an old version.
    • The problem there comes not from the need for ASTHelpers.getSymbol to use baseSymbol() but instead for the need for MethodMatchState.ownerType to look at the actual owner instead of the type of the receiver tree. (If it started to do that, it would then benefit from this change to produce the correct owner.) I added a link to the appropriate bug there.

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.

0 participants