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

False positive finding for WRONG_INDENTATION #1312

Closed
WebTiger89 opened this issue May 27, 2022 · 20 comments · Fixed by #1342
Closed

False positive finding for WRONG_INDENTATION #1312

WebTiger89 opened this issue May 27, 2022 · 20 comments · Fixed by #1342
Assignees
Labels
bug Something isn't working
Milestone

Comments

@WebTiger89
Copy link

WebTiger89 commented May 27, 2022

When running diktatCheck I get a false-positive finding for the following code snippet:

            Surface(
                modifier = Modifier.fillMaxSize(),
                color = MaterialTheme.colors.background,
            ) {
                Text("Android")
            }

Warning: expected 20 but was 16
So it expects:

            Surface(
                    modifier = Modifier.fillMaxSize(),
                    color = MaterialTheme.colors.background,
            ) {
                Text("Android")
            }

Maybe this is Jetpack Compose specific and you want to adjust the rule in this direction.

@WebTiger89 WebTiger89 added the bug Something isn't working label May 27, 2022
@orchestr7
Copy link
Member

Thank you for you report! It is very confusing, but it should be working. We will check asap!

@orchestr7
Copy link
Member

orchestr7 commented May 27, 2022

@WebTiger89 hm, I checked it in our online demo and it works fine...

image

@orchestr7
Copy link
Member

Which version are you using? May be code snippet is more complex?

@WebTiger89
Copy link
Author

WebTiger89 commented May 27, 2022

Damn I checked the diktat-analysis.yml file and by default extendedIndentOfParameters is true

    # If true: in parameter list when parameters are split by newline they are indented with two indentations instead of one
    extendedIndentOfParameters: true

When I disable it, it works at this point like expected :)

EDIT: I'm on the latest version

@WebTiger89
Copy link
Author

@WebTiger89
Copy link
Author

You might want to adjust the default setting. Can be closed. Thanks for the fast support :)

@orchestr7
Copy link
Member

orchestr7 commented May 27, 2022

extendedIndentOfParameters

yeah, that is the legacy support for old IDEAs :)
There was a bug, that put 2 indents instead of one. They have fixed it.

@orchestr7
Copy link
Member

You might want to adjust the default setting.
Exactly! Thank you for catching that! We forgot to revert an example

@unix-junkie unix-junkie removed their assignment May 27, 2022
0x6675636b796f75676974687562 pushed a commit that referenced this issue May 27, 2022
…s` to `false`

### What's done:

 * The `WRONG_INDENTATION:extendedIndentOfParameters` flag is now unset by default. This closes #1312.
0x6675636b796f75676974687562 added a commit that referenced this issue May 27, 2022
…s` to `false`

### What's done:

 * The `WRONG_INDENTATION:extendedIndentOfParameters` flag is now unset by default. This closes #1312.
0x6675636b796f75676974687562 pushed a commit that referenced this issue May 27, 2022
…s` to `false`

### What's done:

 * The `WRONG_INDENTATION:extendedIndentOfParameters` flag is now unset by default. This closes #1312.
0x6675636b796f75676974687562 added a commit that referenced this issue May 27, 2022
…s` to `false`

### What's done:

 * The `WRONG_INDENTATION:extendedIndentOfParameters` flag is now unset by default. This closes #1312.
@WebTiger89
Copy link
Author

WebTiger89 commented May 27, 2022

@akuleshov7 Sorry I have to reopen this. While example above is not reported anymore after setting extendedIndentOfParameters: false in config file, everything else is still reported. When I opened the issue I checked only a small part of the code but now I'm running the check over hundred of files so I noticed it only now.

Example:

@Composable
fun YoutubeChip(selected: Boolean, text: String, modifier: Modifier = Modifier) {
    Surface(
        color = when {
            selected -> MaterialTheme.colors.onSurface
            else -> Color.Transparent
        },
    )
}

[WRONG_INDENTATION] only spaces are allowed for indentation and each indentation should equal to 4 spaces (tabs are not allowed): expected 12 but was 8

etc.

Ktlint online reports it too.

Here the whole rule of my config file:

# Checks that indentation is correct
- name: WRONG_INDENTATION
  enabled: true
  configuration:
    # Is newline at the end of a file needed
    newlineAtEnd: true
    # If true: in parameter list when parameters are split by newline they are indented with two indentations instead of one
    extendedIndentOfParameters: false
    # If true: if first parameter in parameter list is on the same line as opening parenthesis, then other parameters can be aligned with it
    alignedParameters: true
    # If true: if expression is split by newline after operator like +/-/`*`, then the next line is indented with two indentations instead of one
    extendedIndentAfterOperators: true
    # The indentation size for each file
    indentationSize: 4

To be honest it does not make any sense why the first example is not reported but this one is. It happens everywhere in code, not only Jetpack Compose code. If you need more examples just let me know.

@WebTiger89
Copy link
Author

WebTiger89 commented May 28, 2022

I might figured out on which code snippets it fails mainly. It seems 2 specific cases can not be handled properly:

  1. Expression bodies
  2. Jetpack Compose Modifier moved to next line

In these cases diktatCheck reports WRONG_INDENTATION and would fix it by adding 4 extra indents to the lines below the line with the '='.

Example for 1:

 val currentTime: Time
    get() =
            with(currentDateTime) {
                Time(hour = hour, minute = minute, second = second)
            }

or

fun formatDateByPattern(date: String, pattern: String = "ddMMyy"): String =
    DateTimeFormatter.ofPattern(pattern).format(LocalDate.parse(date))

or

fun LocalDateTime.updateTime(
    hour: Int? = null,
    minute: Int? = null,
    second: Int? = null,
): LocalDateTime = withHour(hour ?: getHour())
    .withMinute(minute ?: getMinute())
    .withSecond(second ?: getSecond())

or

    private fun createLayoutParams(): WindowManager.LayoutParams =
        WindowManager.LayoutParams().apply {
            type = WindowManager.LayoutParams.TYPE_APPLICATION_PANEL
            token = composeView.applicationWindowToken
            width = WindowManager.LayoutParams.MATCH_PARENT
            height = WindowManager.LayoutParams.MATCH_PARENT
            format = PixelFormat.TRANSLUCENT

            // TODO make composable configurable

            // see https://stackoverflow.com/questions/43511326/android-making-activity-full-screen-with-status-bar-on-top-of-it
            if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.R) {
                windowInsetsController?.hide(WindowInsets.Type.statusBars())
            } else {
                @Suppress("DEPRECATION")
                systemUiVisibility = (View.SYSTEM_UI_FLAG_LOW_PROFILE
                        or View.SYSTEM_UI_FLAG_FULLSCREEN
                        or View.SYSTEM_UI_FLAG_LAYOUT_STABLE
                        or View.SYSTEM_UI_FLAG_IMMERSIVE_STICKY
                        or View.SYSTEM_UI_FLAG_LAYOUT_HIDE_NAVIGATION
                        or View.SYSTEM_UI_FLAG_HIDE_NAVIGATION)
            }
        }

or

    val offsetDelta =
        if (shimmerAnimationType != ShimmerAnimationType.FADE) translateAnim.dp
        else 2000.dp

or

private fun lerp(start: Float, stop: Float, fraction: Float): Float =
    (1 - fraction) * start + fraction * stop

Example for 2:

Box(
	Modifier
		.align(Alignment.CenterVertically)
		.widthIn(min = 16.dp + 40.dp),
	contentAlignment = Alignment.CenterStart,
)

Note: Git won't indent last example properly, but you know how it should look like.

@0x6675636b796f75676974687562
Copy link
Member

@WebTiger89, thank you for your feedback.

I'm looking into the issue and will get back to you in a short while.

0x6675636b796f75676974687562 added a commit that referenced this issue May 30, 2022
… if there's no config file

### What's done:

 * The default value of `extendedIndentOfParameters` is now `false` even if the YAML config file is missing.
 * Related to #1312 gh-1312.
0x6675636b796f75676974687562 added a commit that referenced this issue May 30, 2022
… if there's no config file (#1329)

### What's done:

 * The default value of `extendedIndentOfParameters` is now `false` even if the YAML config file is missing.
 * Related to #1312.
@orchestr7 orchestr7 changed the title false positive finding for WRONG_INDENTATION on Jetpack Compose code False positive finding for WRONG_INDENTATION May 30, 2022
@orchestr7
Copy link
Member

orchestr7 commented May 30, 2022

fun formatDateByPattern(date: String, pattern: String = "ddMMyy"): String =
DateTimeFormatter.ofPattern(pattern).format(LocalDate.parse(date))

code provided by @WebTiger89 is a legacy issue in IDEA - I have described it here: #1323 (comment)
They have partially fixed it and we need to support this logic

@0x6675636b796f75676974687562
Copy link
Member

0x6675636b796f75676974687562 commented May 30, 2022

@WebTiger89, today we've made a minor fix (0706885) which may affect your experience. From now on, extendedIndentOfParameters defaults to false even if there's no config file available.

Apart from that, your first example is correct:

@Composable
fun YoutubeChip(selected: Boolean, text: String, modifier: Modifier = Modifier) {
    Surface(
        color = when {
            selected -> MaterialTheme.colors.onSurface
            else -> Color.Transparent
        },
    )
}

— and consistent with the indentation config you've provided:

- name: WRONG_INDENTATION
  enabled: true
  configuration:
    newlineAtEnd: true
    extendedIndentOfParameters: false
    alignedParameters: true
    extendedIndentAfterOperators: true
    indentationSize: 4

If you change the value of extendedIndentOfParameters to true (the previous behaviour), the code would need to be reformatted to become

@Composable
fun YoutubeChip(selected: Boolean, text: String, modifier: Modifier = Modifier) {
    Surface(
            color = when {
                selected -> MaterialTheme.colors.onSurface
                else -> Color.Transparent
            },
    )
}

— note that I've increased the indentation in lines 4 through 7: the 4-line color expressions is a function parameter and needs to be indented.

Additional examples you've supplied are affected by other flags in the WRONG_INDENTATION group:

  • alignedParameters,
  • extendedIndentAfterOperators, and
  • extendedIndentBeforeDot.

I'll provide the detailed feedback for each example tomorrow morning. Yet, the way DiKTat's indentation checker behaves looks good to me, I don't see any issues so far.

@0x6675636b796f75676974687562 0x6675636b796f75676974687562 added this to the 1.1.1 milestone May 31, 2022
@0x6675636b796f75676974687562
Copy link
Member

Expression bodies, example 1 of 6:

val currentTime: Time
    get() =
            with(currentDateTime) {
                Time(hour = hour, minute = minute, second = second)
            }

— unaffected by the extendedIndentOfParameters flag (as well as alignedParameters and extendedIndentBeforeDot).

If you want to un-set the extendedIndentAfterOperators (which is on by default), the code needs to be reformatted as follows in order to comply:

val currentTime: Time
    get() =
        with(currentDateTime) {
            Time(hour = hour, minute = minute, second = second)
        }

@0x6675636b796f75676974687562
Copy link
Member

Expression bodies, example 2 of 6:

fun formatDateByPattern(date: String, pattern: String = "ddMMyy"): String =
    DateTimeFormatter.ofPattern(pattern).format(LocalDate.parse(date))

— same as the previous example, only affected by the extendedIndentAfterOperators (needs to be un-set for the formatting to be compliant). If you set the flag or use its default value, be sure to reformat the fragment:

fun formatDateByPattern(date: String, pattern: String = "ddMMyy"): String =
        DateTimeFormatter.ofPattern(pattern).format(LocalDate.parse(date))

@0x6675636b796f75676974687562
Copy link
Member

0x6675636b796f75676974687562 commented May 31, 2022

Expression bodies, example 3 of 6:

fun LocalDateTime.updateTime(
    hour: Int? = null,
    minute: Int? = null,
    second: Int? = null,
): LocalDateTime = withHour(hour ?: getHour())
    .withMinute(minute ?: getMinute())
    .withSecond(second ?: getSecond())

— with the current formatting compliant with extendedIndentOfParameters set to off, but not compliant with the extendedIndentBeforeDot flag. If you set extendedIndentBeforeDot to on (the default), the formatting should be:

fun LocalDateTime.updateTime(
    hour: Int? = null,
    minute: Int? = null,
    second: Int? = null,
): LocalDateTime = withHour(hour ?: getHour())
        .withMinute(minute ?: getMinute())
                .withSecond(second ?: getSecond())

With extendedIndentBeforeDot set to off:

fun LocalDateTime.updateTime(
    hour: Int? = null,
    minute: Int? = null,
    second: Int? = null,
): LocalDateTime = withHour(hour ?: getHour())
    .withMinute(minute ?: getMinute())
        .withSecond(second ?: getSecond())

The expected indentation level being incremented with each new chained method call is a bug and will be fixed separately (see gh-1336).

@0x6675636b796f75676974687562
Copy link
Member

0x6675636b796f75676974687562 commented May 31, 2022

Expression bodies, example 4 of 6:

example4-original

DiKTat reports WRONG_INDENTATION errors in lines 17-21 regardless of the extendedIndentAfterOperators flag because you're wrapping the expression before the or operator (or, more precisely, infix function), while wrapping it after the operator is the recommended approach.

Wrapping expressions before an operator in Kotlin is discouraged, partially because the same operators may be used for both unary and binary operations, e. g.:

    fun foo() {
        val a = 3 +
                2   // a = 5

        val b = 3   // b = 3
                + 2 // and this is just an unary plus: 2.unaryPlus()
    }

For the same reason, DiKTat will additionally report WRONG_NEWLINES errors in the same lines (17-21) if this inspection is enabled. I suggest that you reformat your code as follows:

    private fun createLayoutParams(): WindowManager.LayoutParams =
        WindowManager.LayoutParams().apply {
            type = WindowManager.LayoutParams.TYPE_APPLICATION_PANEL
            token = composeView.applicationWindowToken
            width = WindowManager.LayoutParams.MATCH_PARENT
            height = WindowManager.LayoutParams.MATCH_PARENT
            format = PixelFormat.TRANSLUCENT

            // TODO make composable configurable

            // see https://stackoverflow.com/questions/43511326/android-making-activity-full-screen-with-status-bar-on-top-of-it
            if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.R) {
                windowInsetsController?.hide(WindowInsets.Type.statusBars())
            } else {
                @Suppress("DEPRECATION")
                systemUiVisibility = (View.SYSTEM_UI_FLAG_LOW_PROFILE or
                        View.SYSTEM_UI_FLAG_FULLSCREEN or
                        View.SYSTEM_UI_FLAG_LAYOUT_STABLE or
                        View.SYSTEM_UI_FLAG_IMMERSIVE_STICKY or
                        View.SYSTEM_UI_FLAG_LAYOUT_HIDE_NAVIGATION or
                        View.SYSTEM_UI_FLAG_HIDE_NAVIGATION)
            }
        }

You will be able to control whether a regular (single) or a continuation (double) indent level is used using the extendedIndentAfterOperators flag (on by default). Still, the behaviour you're observing is indeed a bug; reported as #1340.

At the same time, there's indeed an issue with the way how DiKTat checks expression bodies which start at the next line. Should be (IDEA 2022.1 with default code style settings, using continuation indent for expression body functions):

    private fun createLayoutParams(): WindowManager.LayoutParams =
            WindowManager.LayoutParams().apply { /* ... */ }

DiKTat, extendedIndentAfterOperators flag on:

    private fun createLayoutParams(): WindowManager.LayoutParams =
        WindowManager.LayoutParams().apply { /* ... */ }

DiKTat, extendedIndentAfterOperators flag off, outright wrong:

    private fun createLayoutParams(): WindowManager.LayoutParams =
    WindowManager.LayoutParams().apply { /* ... */ }

The same is true (continuation indent should be used) for other examples you've submitted:

    val offsetDelta =
        if (shimmerAnimationType != ShimmerAnimationType.FADE) translateAnim.dp
        else 2000.dp

and

private fun lerp(start: Float, stop: Float, fraction: Float): Float =
    (1 - fraction) * start + fraction * stop

I believe this is gh-1330.

@0x6675636b796f75676974687562
Copy link
Member

0x6675636b796f75676974687562 commented Jun 1, 2022

@WebTiger89, your other examples:

box

and

surface

are well-formatted provided indentationSize is 4 and both extendedIndentOfParameters and extendedIndentBeforeDot are on. IDEA code style counterparts are CONTINUATION_INDENT_IN_ARGUMENT_LISTS and CONTINUATION_INDENT_FOR_CHAINED_CALLS, respectively. As far as I see, DiKTat (both 1.1.0 and 1.1.1-SNAPSHOT) reports no errors against these code fragments: the fragments have no expression body functions and hence are not subjected to #1336.

@0x6675636b796f75676974687562
Copy link
Member

0x6675636b796f75676974687562 commented Jun 1, 2022

@akuleshov7, I suggest we switch the default value of extendedIndentOfParameters back to true. As per the Kotlin code style guidelines, all relevant code style flags default to true in IDEA:

  • CONTINUATION_INDENT_IN_PARAMETER_LISTS
  • CONTINUATION_INDENT_IN_ARGUMENT_LISTS
  • CONTINUATION_INDENT_FOR_EXPRESSION_BODIES
  • CONTINUATION_INDENT_FOR_CHAINED_CALLS
  • CONTINUATION_INDENT_IN_SUPERTYPE_LISTS
  • CONTINUATION_INDENT_IN_IF_CONDITIONS
  • CONTINUATION_INDENT_IN_ELVIS

Update: the above is not true, see below.

@0x6675636b796f75676974687562
Copy link
Member

0x6675636b796f75676974687562 commented Jun 2, 2022

The previous comment turned out to be not true. IDEA behaves differently, depending on how exactly Kotlin style guide is applied.

By default (i. e. unless any style guide is applied, empty XML file), continuation indent is on wherever applicable, equivalent to:

    <JetCodeStyleSettings>
      <option name="CONTINUATION_INDENT_IN_PARAMETER_LISTS" value="true" />
      <option name="CONTINUATION_INDENT_IN_ARGUMENT_LISTS" value="true" />
      <option name="CONTINUATION_INDENT_FOR_EXPRESSION_BODIES" value="true" />
      <option name="CONTINUATION_INDENT_FOR_CHAINED_CALLS" value="true" />
      <option name="CONTINUATION_INDENT_IN_SUPERTYPE_LISTS" value="true" />
      <option name="CONTINUATION_INDENT_IN_IF_CONDITIONS" value="true" />
      <option name="CONTINUATION_INDENT_IN_ELVIS" value="true" />
    </JetCodeStyleSettings>

Now, if the code style is set via Preferences -> Editor -> Code Style -> Kotlin -> Load/Save -> Use defaults from: -> Kotlin Coding Conventions:

idea-code-style-0

— then the following settings are explicitly set in XML:

    <JetCodeStyleSettings>
      <option name="CONTINUATION_INDENT_IN_PARAMETER_LISTS" value="true" />
      <option name="CONTINUATION_INDENT_IN_ARGUMENT_LISTS" value="true" />
      <option name="CONTINUATION_INDENT_FOR_EXPRESSION_BODIES" value="true" />
      <option name="CONTINUATION_INDENT_FOR_CHAINED_CALLS" value="true" />
      <option name="CONTINUATION_INDENT_IN_SUPERTYPE_LISTS" value="true" />
      <option name="CONTINUATION_INDENT_IN_IF_CONDITIONS" value="true" />
      <option name="CONTINUATION_INDENT_IN_ELVIS" value="true" />
      <option name="CODE_STYLE_DEFAULTS" value="KOTLIN_OFFICIAL" />
    </JetCodeStyleSettings>

Note that CODE_STYLE_DEFAULTS is set to KOTLIN_OFFICIAL, and all CONTINUATION_INDENT_* flags are explicitly set to true.

Next, if we re-set our code style settings to the defaults and re-apply the same Kotlin style guide, now from the "Set from..." hyperlink:

idea-code-style-1

— then .idea/codeStyles/Project.xml will have the following content:

    <JetCodeStyleSettings>
      <option name="CODE_STYLE_DEFAULTS" value="KOTLIN_OFFICIAL" />
    </JetCodeStyleSettings>

— and the effective values of all continuation indent related flags will be false.

That said, IDEA behaviour is inconsistent and misleading, and the official style guide (in which there's not a single mention of a continuation indent) should be consulted from now on.

See also Differences between "Kotlin coding conventions" and "IntelliJ IDEA default code style":

The most notable change is in the continuation indentation policy. There's a nice idea to use the double indent for showing that a multi-line expression hasn't ended on the previous line. This is a very simple and general rule, but several Kotlin constructions look a bit awkward when they are formatted this way. In Kotlin coding conventions, it's recommended to use a single indent in cases where the long continuation indent has been forced before.

Kotlin style guide for Android prescribes exactly the same:

When a function signature does not fit on a single line, break each parameter declaration onto its own line. Parameters defined in this format should use a single indent (+4). The closing parenthesis ()) and return type are placed on their own line with no additional indent.

0x6675636b796f75676974687562 added a commit that referenced this issue Jun 2, 2022
…dIndentBeforeDot` to `false`

### What's done:

 * Since Kotlin style guide mentions no continuation indent,
   and IDEA defaults to no continuation indent as well,
   all `extendedIndent*` flags default to `false` from now on.
 * Closes: #1312
0x6675636b796f75676974687562 added a commit that referenced this issue Jun 3, 2022
…dIndentBeforeDot` to `false`

### What's done:

 * Since Kotlin style guide mentions no continuation indent,
   and IDEA defaults to no continuation indent as well,
   all `extendedIndent*` flags default to `false` from now on.
 * Closes: #1312
0x6675636b796f75676974687562 added a commit that referenced this issue Jun 3, 2022
…dIndentBeforeDot` to `false` (#1342)

### What's done:

 * Since Kotlin style guide mentions no continuation indent,
   and IDEA defaults to no continuation indent as well,
   all `extendedIndent*` flags default to `false` from now on.
 * Reformatted the codebase by running `mvn diktat:fix@diktat` so that it conforms to the new rules.
 * Unit tests adapted to the reformatted test resources.
 * Unit tests adapted to the new defaults.
 * Closes: #1312
@nulls nulls modified the milestones: 1.2.0, 1.1.1 Jun 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
5 participants