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.

Sign up for GitHub

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

Jump to bottom

Added type validation to Assertions. #543

Merged
merged 1 commit into from
Dec 13, 2023
Merged

Added type validation to Assertions. #543

merged 1 commit into from
Dec 13, 2023

Conversation

traceyyoshima
Copy link
Contributor

@traceyyoshima traceyyoshima commented Dec 13, 2023 •
edited
Loading

Changes:

  • Kotlin test harness will validate types.

fixes #482
fixes #474

@traceyyoshima
Copy link
Contributor Author

traceyyoshima commented Dec 13, 2023 •
edited
Loading

Resolved: Blocked by #480.
In J, switch expressions add the default keyword as an expression too. The else keyword in a Kotlin when expression is functionally the same. The FindMissingTypes recipe used for TypeValidation has a condition that accepts default and other case statements with null types. So, a similar condition has been added to validate Kotlin types in the LST.

traceyyoshima
traceyyoshima commented Dec 13, 2023
View reviewed changes
src/main/java/org/openrewrite/kotlin/Assertions.java
@@ -196,6 +209,36 @@ public static UncheckedConsumer<SourceSpec<?>> spaceConscious() {
};
}

private static void assertValidTypes(TypeValidation typeValidation, J sf) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@knutwannheden @kunli2, I don't think mixins will help us customize the FindMissingTypes recipe. Initially, I thought about creating a FindMissingKotlinTypes recipe/visitor, which could arguably be helpful. But I recall Jonathan preferring nearly identical recipes not to be created. Eventually, if we find there is a need for the augmented recipe, we can move the code.

What do you think? Is it fine to have the visitor in Assertions for now or would you prefer a recipe?

Copy link
Contributor

Choose a reason for hiding this comment

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

At some point I would be interested in the details regarding the mixins, do that we can try to come up with a fix or another approach for this. An alternative might be delegation kind of like we do in the printer.

For now I think what you currently have is good. Having type validations working is really a step forward. Thanks for fixing this!

src/test/java/org/openrewrite/kotlin/tree/AnnotationTest.java

import java.util.List;

import static org.assertj.core.api.Assertions.assertThat;
import static org.openrewrite.kotlin.Assertions.kotlin;

@SuppressWarnings({"RedundantSuppression", "RedundantNullableReturnType", "RedundantVisibilityModifier", "UnusedReceiverParameter", "SortModifiers", "TrailingComma"})
@SuppressWarnings({"RedundantSuppression", "RedundantNullableReturnType", "RedundantVisibilityModifier", "UnusedReceiverParameter", "SortModifiers", "TrailingComma", "RedundantGetter", "RedundantSetter"})
class AnnotationTest implements RewriteTest {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I cleaned up the errors/warnings here when I looked through the tests.

@traceyyoshima
Copy link
Contributor Author

@knutwannheden type validation is essentially ready for review. The failing tests are identifiers with aliases, and I'm looking into how to detect aliases.

import java.util.regex.Pattern.CASE_INSENSITIVE as i

class A {
    val f = arrayOf(i)
}

knutwannheden
knutwannheden approved these changes Dec 13, 2023
View reviewed changes
Copy link
Contributor

@knutwannheden knutwannheden left a comment

Choose a reason for hiding this comment

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

As we can disable validations on a test case basis, we can fix any remaining issues in follow up PRs.

@traceyyoshima
Added type validation to Assertions.
25bf1ca 
Disabled type validation on tests with intentionally unresolvable types.
Fixed NPE in KotlinPrinter.
Fixed errors in AnnotationTest.
@traceyyoshima
Copy link
Contributor Author

traceyyoshima commented Dec 13, 2023 •
edited
Loading

As we can disable validations on a test case basis, we can fix any remaining issues in follow up PRs.

Disabled the alias tests and opened: #545 to track a fix for aliases. Tracking the declarations of a name will allow us to apply the appropriate field type comparison.

@traceyyoshima traceyyoshima marked this pull request as ready for review December 13, 2023 21:35
@traceyyoshima traceyyoshima merged commit 95dcdea into main Dec 13, 2023
1 check passed
@traceyyoshima traceyyoshima deleted the add-type-validation-to-kotlin branch December 13, 2023 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knutwannheden knutwannheden knutwannheden approved these changes

@kunli2 kunli2 Awaiting requested review from kunli2

Assignees
No one assigned
Labels
None yet
Projects
OpenRewrite
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Info to extend TypeValidation for Kotlin Add type validation to Assertions
2 participants
@traceyyoshima @knutwannheden