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

[FIR] Fix suggesting non-existent InvocationKind #5303

Closed
wants to merge 2 commits into from

Conversation

isuckatcs
Copy link
Contributor

^KT-68291 Fixed

@demiurg906 demiurg906 self-assigned this May 20, 2024
@demiurg906
Copy link
Contributor

Could you also add tests for your change:

  • create a test case in compiler/fir/analysis-tests/testData/resolveWithStdlib/contracts/fromSource/bad/callsInPlace directory
  • write all affected cases in it
// WITH_STDLIB
// ISSUE: KT-68291

import kotlin.contracts.*

@OptIn(ExperimentalContracts::class)
fun test_1(block: () -> Unit) {
    contract {
        callsInPlace(block, InvocationKind.EXACTLY_ONCE)
    }
}
// ... other cases, like with two calls of `block()`
  • run ./gradlew generateTests, so new tests will be generated

It's possible to test diagnostic messages in diagnostic tests:

  1. run generated test
  2. apply the test diff with rendered diagnostic to the testdata file
<!WRONG_INVOCATION_KIND!>callsInPlace(block, InvocationKind.EXACTLY_ONCE)<!>
  1. Manually add ("") to diagnostic marker
<!WRONG_INVOCATION_KIND("")!>callsInPlace(block, InvocationKind.EXACTLY_ONCE)<!>
  1. Run test again. Arguments of diagnostic will be rendered in these quotes
<!WRONG_INVOCATION_KIND("block: () -> Unit; EXACTLY_ONCE; ZERO")!>callsInPlace(block, InvocationKind.EXACTLY_ONCE)<!>

It would be great if there were two commits in this MR: the first one will contain tests, which show the old incorrect behavior. The second contains the fix and update in the testdata, so it can be easily to see the effect of the change

@demiurg906
Copy link
Contributor

Note that you can open nice diff window for failed test by looking for <Click to see difference> button in the stacktrace of failed test or just by pressing Ctrl-D/cmd-D on failed test

@demiurg906
Copy link
Contributor

Everything looks good, I've sent your branch to CI to check all tests
Will merge the MR when it will pass

@demiurg906
Copy link
Contributor

PR merged manually (see b45ce15)
Thank you for contribution!

@demiurg906 demiurg906 closed this May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants