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

Disable extension point org.jetbrains.kotlin.com.intellij.treeCopyHandler #2044

Merged
merged 7 commits into from
May 27, 2023

Conversation

paul-dingemans
Copy link
Collaborator

Description

In Kotlin 1.9 the extension points of the embedded kotlin compiler will change. Ktlint only uses the org.jetbrains.kotlin.com.intellij.treeCopyHandler extension point. This extension is not yet supported in 1.9, neither is it documented (#KT-58704). Without this extension point it might happen that your custom rules will throw exceptions during runtime. See #1981.

In Ktlint, 7 out of 77 rules needed small and sometimes bigger changes to become independent of the extension point org.jetbrains.kotlin.com.intellij.treeCopyHandler. The impact on your custom rules may vary dependent on the way the autocorrect has been implemented. When manipulating ASTNodes there seems to be no impact. When, manipulating PsiElements, some functions consistently result in a runtime exception.

Based on the refactoring of the rules as provided by ktlint-ruleset-standard in Ktlint 0.49.x the suggested refactoring is as follows:

  • Replace LeafElement.replaceWithText(String) with LeafElement.rawReplaceWithText(String).
  • Replace PsiElement.addAfter(PsiElement, PsiElement) with AstNode.addChild(AstNode, AstNode). Note that this method inserts the new node (first) argument before the second argument node and as of that is not a simple replacement of the PsiElement.addAfter(PsiElement, PsiElement).
  • Replace PsiElement.replace(PsiElement) with a sequence of AstNode.addChild(AstNode, AstNode) and AstNode.removeChild(AstNode).

Be aware that your custom rules might use other functions which also throw exceptions when the extension point org.jetbrains.kotlin.com.intellij.treeCopyHandler is no longer supported.

In order to help you to analyse and fix the problems with your custom rules, ktlint temporarily supports to disable the extension point org.jetbrains.kotlin.com.intellij.treeCopyHandler using a flag. This flag is available in the Ktlint CLI and in the KtlintRuleEngine. By default, the extension point is enabled like it was in previous versions of ktlint.

At least you should analyse the problems by running your test suits by running ktlint and disabling the extension point. Next you can start with fixing and releasing the updated rules. All rules in this version of ktlint have already been refactored and are not dependent on the extension point anymore. In Ktlint CLI the flag is to be activated with parameter --disable-kotlin-extension-point. API Consumers that use the KtlintRuleEngine directly, have to set property enableKotlinCompilerExtensionPoint to false.

At this point in time, it is not yet decided what the next steps will be. Ktlint might drop the support of the extension points entirely. Or, if the extension point org.jetbrains.kotlin.com.intellij.treeCopyHandler is fully supported at the time that ktlint will be based on kotlin 1.9 it might be kept. In either case, the flag will be dropped in a next version of ktlint.

Checklist

  • PR description added
  • tests are added
  • KtLint has been applied on source code itself and violations are fixed
  • documentation is updated
  • CHANGELOG.md is updated

In case of adding a new rule:

…n.com.intellij.treeCopyHandler` in the kotlin embeddable compiler to investigate the impact whenever this extension point will not be supported in Kotlin 1.9
…placeWithText(String)`

Up until Kotlin 1.8 the KotlinPsiFactory registered the treeCopyHandler extension point which is needed in order to use the `LeafElement.replaceWithText(String)`. With the Kotlin 1.9 it is no longer possible to register this extension point which results in exception.

The exception does not occur when using method `LeafElement.rawReplaceWithText(String)`.
…ddChild(AstNode, AstNode)`

Up until Kotlin 1.8 the KotlinPsiFactory registered the treeCopyHandler extension point which is needed in order to use the `PsiElement.addAfter(PsiLeaf, PsiLeaf)`. With the Kotlin 1.9 it is no longer possible to register this extension point which results in exception.

The exception does not occur when using method `AstNode.addChild(AstNode, AstNode)`. Note that this method inserts the new node (first) argument *before* the second argument node and as of that is not a simple replacement of the `PsiElement.addAfter(PsiElement, PsiElement)`.
…addChild(AstNode, AstNode)` and `AstNode.removeChild(AstNode)`

Up until Kotlin 1.8 the KotlinPsiFactory registered the treeCopyHandler extension point which is needed in order to use the `PsiElement.replace(PsiElement)`. With the Kotlin 1.9 it is no longer possible to register this extension point which results in exception.
@shashachu
Copy link
Contributor

I'm wondering if it would be possible to log instead of require the user to opt in to use a flag. I'm thinking of Gradle warnings in the form of: `You are using treeCopyHandler. This will be unsupported in Kotlin 1.9.0 ....'

@paul-dingemans
Copy link
Collaborator Author

It is not about opt-in by end users. If kotlin 1.9 does not support the extension point then the rule provider is responsible for making the rule compatible with 1.9 without falling back on this extension point. The idea of this change is that rule providers can check the vulnerability for failures in their rule whenever we upgrade to kotlin 1.9.

For normal users of ktlint this functionality has to be invisible. That is why the Ktlint cli parameter is hidden from the help command.

@paul-dingemans paul-dingemans added this to the 0.49.2 / 0.50.x milestone May 20, 2023
@paul-dingemans paul-dingemans merged commit f431551 into master May 27, 2023
16 checks passed
@paul-dingemans paul-dingemans deleted the 1981-disable-psi-extension-point branch May 27, 2023 18:51
3flex added a commit to 3flex/detekt that referenced this pull request Jul 4, 2023
This was previously used by ktlint to support autocorrect for some rules,
but is being phased out due to pinterest/ktlint#1981.
detekt should do the same. This has no impact on any core rules, or the
formatting module (since ktlint no longer depends on this extension as of
ktlint v0.50.0).

This will impact third party rule sets, only if:
- they support autocorrect, and
- autocorrect support relies on the treeCopyHandler extension

ktlint rules were updated to remove the dependency on treeCopyHandler, so
affected third party detekt rule sets can do the same.

This is all being caused by upstream changes in Kotlin. There's some very
useful analysis in pinterest/ktlint#2044 that can
be reviewed by anyone affected downstream by this change.
3flex added a commit to detekt/detekt that referenced this pull request Jul 10, 2023
* Stop applying treeCopyHandler extension in analysis environment

This was previously used by ktlint to support autocorrect for some rules,
but is being phased out due to pinterest/ktlint#1981.
detekt should do the same. This has no impact on any core rules, or the
formatting module (since ktlint no longer depends on this extension as of
ktlint v0.50.0).

This will impact third party rule sets, only if:
- they support autocorrect, and
- autocorrect support relies on the treeCopyHandler extension

ktlint rules were updated to remove the dependency on treeCopyHandler, so
affected third party detekt rule sets can do the same.

This is all being caused by upstream changes in Kotlin. There's some very
useful analysis in pinterest/ktlint#2044 that can
be reviewed by anyone affected downstream by this change.

* Remove unused test

DetektPomModel is now an object so cannot be instantiated multiple
times, avoiding concurrency issues. The problematic code that was tested
by this test has also been removed.

* Remove unused dependency

contester is not used anymore in any test or production code.
mrmans0n added a commit to mrmans0n/compose-rules that referenced this pull request Sep 9, 2023
Due to the reasons exposed in pinterest/ktlint#2044, this is not trivial - we need to rewrite the fixes manually or remove them altogether. Not being able to rely on the embedded kotlin compiler plugin for this is less than ideal, I wanted to stay away from ASTNode as much as possible :(

TODO:
- [x] Fix spotless version
- [ ] Fix ViewModelInjection autofix
- [ ] Fix PreviewPublic autofix
mrmans0n added a commit to mrmans0n/compose-rules that referenced this pull request Sep 10, 2023
* Update ktlint to 1.0.0

Due to the reasons exposed in pinterest/ktlint#2044, this is not trivial - we need to rewrite the fixes manually or remove them altogether. Not being able to rely on the embedded kotlin compiler plugin for this is less than ideal, I wanted to stay away from ASTNode as much as possible :(

TODO:
- [x] Fix spotless version
- [x] Fix ViewModelInjection autofix
- [x] Fix PreviewPublic autofix

* Fix ComposePreviewPublic autofix

* Use ASTNode for ComposeViewModelInjection autofix
mgroth0 pushed a commit to mgroth0/detekt that referenced this pull request Feb 11, 2024
…kt#6255)

* Stop applying treeCopyHandler extension in analysis environment

This was previously used by ktlint to support autocorrect for some rules,
but is being phased out due to pinterest/ktlint#1981.
detekt should do the same. This has no impact on any core rules, or the
formatting module (since ktlint no longer depends on this extension as of
ktlint v0.50.0).

This will impact third party rule sets, only if:
- they support autocorrect, and
- autocorrect support relies on the treeCopyHandler extension

ktlint rules were updated to remove the dependency on treeCopyHandler, so
affected third party detekt rule sets can do the same.

This is all being caused by upstream changes in Kotlin. There's some very
useful analysis in pinterest/ktlint#2044 that can
be reviewed by anyone affected downstream by this change.

* Remove unused test

DetektPomModel is now an object so cannot be instantiated multiple
times, avoiding concurrency issues. The problematic code that was tested
by this test has also been removed.

* Remove unused dependency

contester is not used anymore in any test or production code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants