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

check for creatable dependencies before generating alternate constructor #258

Merged
merged 1 commit into from
Dec 11, 2023

Conversation

davissuber
Copy link
Contributor

Attempt to recreate the error case in #221

The key ingredient in this change is in ScopeImplFactory - before this change, the ScopeImpl output for the introduced test is:

package testcases.KT007_scope_factory_dependencies

import kotlin.Suppress
import motif.ScopeImpl

@Suppress("REDUNDANT_PROJECTION", "UNCHECKED_CAST")
@ScopeImpl(
  children = [],
  scope = Scope::class,
  dependencies = Scope.Dependencies::class,
)
public class ScopeImpl(
  private val dependencies: Scope.Dependencies,
) : Scope {
  public constructor() : this(object : Scope.Dependencies {})

  public fun scope(): Scope = this
}

Note that this does not compile because Scope.Dependencies has an abstract method.

While determining whether to create a no-arg convenience
constructor, we check for both explicit (creatable) dependencies
in addition to unsatisfied sinks to avoid generating incompatible code

@motif.Scope
interface Scope : Creatable<Scope.Dependencies> {
// note the absence of string() accessor (e.g. sink) in this scope
Copy link
Contributor Author

@davissuber davissuber Nov 22, 2023

Choose a reason for hiding this comment

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

Not having an accessor for string() here is important otherwise the value of getDependencyMethodData(scope).isNotEmpty() in ScopeImplFactory will evaluate to true (which is why the test passed in the original pull request)

In other words, this pull request makes it possible to explicitly declare unused dependencies in motif.

@TonyTangAndroid
Copy link
Contributor

I patched the diff and run the test and confirm that it works as expected. Thank you for your effort in fixing this long standing minor issue.

@TonyTangAndroid TonyTangAndroid merged commit dfb9036 into uber:main Dec 11, 2023
1 check passed
@davissuber davissuber deleted the ds-redo-kapt-test branch December 11, 2023 17:46
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.

2 participants