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

Regression fix: Binds factory method w/ interface #70

Merged
merged 1 commit into from
May 2, 2019
Merged

Regression fix: Binds factory method w/ interface #70

merged 1 commit into from
May 2, 2019

Conversation

Leland-Takamine
Copy link
Collaborator

Support for binding to interface types with binds factory methods.

@Leland-Takamine Leland-Takamine force-pushed the binds-interface branch 3 times, most recently from 177eb99 to d8d96e7 Compare May 2, 2019 04:31
ZacSweers
ZacSweers reviewed May 2, 2019
View reviewed changes
compiler/ast/src/main/kotlin/motif/ast/compiler/CompilerType.kt Outdated
@@ -81,9 +81,12 @@ class CompilerType(
return type
}

val superType = MoreTypes.nonObjectSuperclass(env.typeUtils, env.elementUtils, type).orNull() ?: return null
env.typeUtils.directSupertypes(type).forEach { superType ->
getMatchingSuperType(baseType, superType as DeclaredType)?.let { return it }
Copy link

Choose a reason for hiding this comment

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

Can this cast fail?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes for error types but that assumption is made across the codebase. To handle error types correctly a more holistic approach is needed.

Copy link

Choose a reason for hiding this comment

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

Link a followup issue?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#51

compiler/ast/src/main/kotlin/motif/ast/compiler/CompilerType.kt Outdated
@@ -81,9 +81,12 @@ class CompilerType(
return type
}

val superType = MoreTypes.nonObjectSuperclass(env.typeUtils, env.elementUtils, type).orNull() ?: return null
env.typeUtils.directSupertypes(type).forEach { superType ->
Copy link

@ZacSweers ZacSweers May 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This could be simplified and made safer to

return env.typeUtils.directSupertypes(type)
    .asSequence()
    .filterInstance<DeclaredType>()
    .mapNotNull { getMatchingType(baseType, it) }
    .firstOrNull()

Copy link

Choose a reason for hiding this comment

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

Also - is this necessary? On mobile but there is definitely some API on Types like isAssignable(Type from, Type to)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We actually don't want to filter out DeclaredTypes here. We should fail fast instead, so going to keep the cast until we have a holistic error type strategy. Will make the sequence change though.

Copy link

Choose a reason for hiding this comment

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

You could move that cast into the mapNotNull body then and remove the filter

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated

tests/src/main/java/testcases/T052_binds_interface_generic/Test.java
@@ -0,0 +1,26 @@
/*
* Copyright (c) 2018 Uber Technologies, Inc.
Copy link

Choose a reason for hiding this comment

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

2019 here and elsewhere

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will do this in a follow up

ZacSweers
ZacSweers reviewed May 2, 2019
View reviewed changes
Copy link

@ZacSweers ZacSweers left a comment

Choose a reason for hiding this comment

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

Definitely think there should be a wildcard test/coverage if you want this to play nice with Kotlin, which contains wildcards by default in bytecode

tests/src/main/java/testcases/T052_binds_interface_generic/A.java
*/
package testcases.T052_binds_interface_generic;

public class A<T> implements B<T> {}
Copy link

Choose a reason for hiding this comment

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

What happens with wildcards?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will add tests

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wildcards aren't valid here - what do you mean?

@Leland-Takamine
Copy link
Collaborator Author

Leland-Takamine commented May 2, 2019 •
edited
Loading

I assume you mean bounded type parameters right?

@Leland-Takamine
Copy link
Collaborator Author

Updated with tests for bounded type parameters and wildcard return types / parameter types.

@Leland-Takamine Leland-Takamine merged commit c013d87 into master May 2, 2019
@Leland-Takamine Leland-Takamine deleted the binds-interface branch May 2, 2019 05:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZacSweers ZacSweers ZacSweers left review comments

@Ericliu001 Ericliu001 Ericliu001 approved these changes

@kurtisnelson kurtisnelson Awaiting requested review from kurtisnelson

@tyvsmith tyvsmith Awaiting requested review from tyvsmith

@dmitry-zaitsev dmitry-zaitsev Awaiting requested review from dmitry-zaitsev

@jbarr21 jbarr21 Awaiting requested review from jbarr21

@cryptogrammer cryptogrammer Awaiting requested review from cryptogrammer

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Leland-Takamine @ZacSweers @Ericliu001