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

Actually support records #1414

Closed
wants to merge 1 commit into from
Closed

Actually support records #1414

wants to merge 1 commit into from

Conversation

ferozco
Copy link
Contributor

@ferozco ferozco commented Jun 12, 2020

Before this PR

I accidentally pushed and merged a test case with a suppression that broke the test: #1412.

After this PR

==COMMIT_MSG==
Actually support records with StrictUnusedVariable
==COMMIT_MSG==

Possible downsides?

N/A

@changelog-app
Copy link

changelog-app bot commented Jun 12, 2020

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Actually support records with StrictUnusedVariable

Check the box to generate changelog(s)

  • Generate changelog entry

@policy-bot policy-bot bot requested a review from iamdanfox June 12, 2020 18:53
isSubtype(getType(tree), Suppliers.typeFromString(t).get(state), state))) {

Pattern isRecord = Pattern.compile("record\\s+" + tree.getSimpleName() + "\\(");
if (isRecord.matcher(state.getSourceForNode(tree)).find()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This could become expensive, I wonder if we can get away with state.getSourceForNode(tree).startsWith("record ")?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately records can have a visibility modifier so that check doesn't quite work

.addSourceLines(
"Test.java",
"class Test {",
"@SuppressWarnings(\"StrictUnusedVariable\")",
Copy link
Contributor

Choose a reason for hiding this comment

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

😂

@iamdanfox
Copy link
Contributor

So are we saying that new error-prone doesn't actually use a modern java compiler that would allow us to use first-class methods to determine if an AST is actually a record?? It does seem very sad to have to regex things :(

@ferozco
Copy link
Contributor Author

ferozco commented Jun 15, 2020

Yes its pretty unfortunate to have to do a regex. We could leave things as is and say that we don't have 100% support for record but that you at least now have an escape hatch with @SuppressWarnings

@ferozco
Copy link
Contributor Author

ferozco commented Jun 18, 2020

I'm going to close this out and say that we only partially support records at the given moment, which seems reasonable given that they are still a preview feature

@ferozco ferozco closed this Jun 18, 2020
@robert3005 robert3005 deleted the fo/more-records branch November 18, 2020 20:06
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.

3 participants