Reject default arguments on composable contract members - #85
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Splitting
GitPanelApiinto a contract module shipped a crash:The cause was default argument values on a
@Composableinterface member. The Compose compiler emits aComposeDefaultImpls.<name>$defaultbridge that calls the abstract method through a synthesised signature; while the interface and its implementation shared a module that resolved, and once they were split it did not.It compiled, passed detekt, and passed all 714 unit tests. It was only found by opening the panel on an emulator.
That was one contract module. The plan is to give every feature one, and both reference architectures this is modelled on put
@Composableentry points in exactly those modules — so this is the shape the codebase is about to adopt seven more times. A rule the build enforces is worth more than a note in a review checklist.What changed
verifyModuleBoundariesnow also scans the Kotlin sources of every:*:apimodule and fails on a@Composableinterface member declared with default argument values, pointing at the file and line and explaining the failure mode.The detection lives in
ApiContractRules.ktas pure Kotlin with no Gradle types, matching how the dependency rules are written, so it is directly unit testable.Behavior
No behavior change. One additional check in an existing verification task, already wired into CI.
Tests
Nine unit tests. Three cover detection, including the exact multi-line declaration that shipped the crash. The other six pin the cases that must not be reported, since a source-level check earns its keep only if it is quiet:
@Composableinterface member with defaults — those do not go through the bridge and are safe, e.g.ActiveBuildRepository.clear(buildId: String? = null)@Composablewith defaults, which is ordinary and correctdata classdefault value, e.g.BuildClientMeta>=inside a signature is not read as a defaultThe guard was verified against the real bug, not just against fixtures: reintroducing
onOpenHistory: () -> Unit = {}intoGitPanelApimakes the build fail with the new message, and removing it makes it pass again.Verification
./gradlew -p build-logic test— 9 tests, 0 failures./gradlew test detekt verifyModuleBoundaries :app:assembleDebug— BUILD SUCCESSFUL