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

Escape soft and modifier keywords #994

Merged
merged 1 commit into from
Oct 9, 2020

Conversation

Egorand
Copy link
Collaborator

@Egorand Egorand commented Oct 8, 2020

Fixes #991

Feels overly restrictive, but the only alternative would be to encode the compiler rules of allowing soft keywords in specific contexts.

@Egorand
Copy link
Collaborator Author

Egorand commented Oct 8, 2020

Failure is due to ktlint, my guess is that it doesn't like trailing commas. Will see if there's a newer version, otherwise will remove the comma.

@Egorand Egorand force-pushed the egorand/201008/escape-all-keywords branch from 4105e8c to 564dd59 Compare October 8, 2020 15:08
@Egorand Egorand changed the base branch from master to egorand/201008/ktlint-0.39.0 October 8, 2020 15:09
Base automatically changed from egorand/201008/ktlint-0.39.0 to master October 8, 2020 15:09
@ZacSweers
Copy link
Collaborator

I'm uneasy about this to be honest. Could we scope this down at all to only specific problematic spots?

Copy link
Collaborator

@JakeWharton JakeWharton left a comment

Choose a reason for hiding this comment

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

I'm fine with this. Fix the problem, and then pull it back where we can. Better than playing whack-a-mole long-term.

@Egorand
Copy link
Collaborator Author

Egorand commented Oct 9, 2020

Could we scope this down at all to only specific problematic spots?

Since there's an unknown number of problematic spots (enum value with constructor arguments is just an example) I think we'll be in a better spot if we catch all potential problems. Agree this is not needed in a lot of constructs, but maybe we can figure out a way to allow-list constructs that only require escaping the hard keywords? Open to ideas, but gonna merge this for now as it fixes the bug.

@Egorand Egorand merged commit 13d9794 into master Oct 9, 2020
@Egorand Egorand deleted the egorand/201008/escape-all-keywords branch October 9, 2020 14:42
Copy link
Member

@oldergod oldergod left a comment

Choose a reason for hiding this comment

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

thank you

@@ -165,36 +165,87 @@ private val IDENTIFIER_REGEX =

internal val String.isIdentifier get() = IDENTIFIER_REGEX.matches(this)

// https://github.com/JetBrains/kotlin/search?q=KeywordStringsGenerated.java
// https://kotlinlang.org/docs/reference/keyword-reference.html
Copy link
Member

Choose a reason for hiding this comment

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

👍

private val KEYWORDS = setOf(
"package",
// Hard keywords
Copy link
Member

Choose a reason for hiding this comment

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

@abakes
Copy link

abakes commented Nov 10, 2020

@Egorand I was wondering if you could expand on what this does exactly? This seems to have broken one of my clients

@Egorand
Copy link
Collaborator Author

Egorand commented Nov 10, 2020

@abakes we used to only escape hard keywords before this change, but the issue this PR fixes described a scenario where that approach was failing, hence the decision to start escaping all Kotlin keywords. Wanna open a new issue and describe what broke in your code? A runnable test case would be ideal.

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.

Modified keywords not escape when generating enums.
5 participants