Re: [PATCH v5 03/10] dtc: dt-check-style: Fix alignment of values in continued property lines

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] String index calculated from stripped string is used to slice the unstripped string, causing incorrect column alignment if a comment precedes the first '<' or '"'. The `check_continuation_alignment` function uses `_strip_strings_and_comments` to strip comments from `rest`, reducing its length. It then runs `re.search` on `rest` to find `<` or `"`. The resulting match index (`m.start(1)`) is then incorrectly used to slice the original unstripped `dl.raw` string. If a block comment exists between `=` and the first value, the slice truncates prematurely, discarding the visual width of the comment and resulting in a `target_col` that is too small.
- [Medium] Incomplete detection of closed values for space-separated arrays and concatenated strings causes false positive alignment errors. The `check_continuation_alignment` function tracks whether a value block is closed using `endswith('",') or endswith('>,')`. This strict check assumes that completed values must end with a comma. In valid DTS, cell arrays or strings can be concatenated across lines without commas (e.g., `ranges = <...>
<...>;` or `prop = "first"
"second";`). If a line ends with `>` or `"` without a comma, the block is closed, but `dl_value_complete` incorrectly evaluates to `False`. The script then unconditionally applies `target_offset = 1` to the subsequent line, improperly demanding that the new `<...>` or string be indented one space to the right (aligned with inner values) rather than aligning with the leading `<` or `"`.
--

commit 3c5fcc59a610ff8a5dc6318a90c848ba441566f3
Author: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>

dtc: dt-check-style: Fix alignment of values in continued property lines

Continued lines in property assignments should be indented to opening
'<' or '"' if they also start with that character. If the continued line
is part of previous phandle, then alignment should be to inner values to
make it more readable.

> diff --git a/scripts/dtc/dt-check-style b/scripts/dtc/dt-check-style
> --- a/scripts/dtc/dt-check-style
> +++ b/scripts/dtc/dt-check-style
> @@ -938,17 +939,26 @@ def check_continuation_alignment(ctx):
>          eq = dl.raw.find('=')
>          if eq < 0:
>              continue
> -        # First '<' or '"' after '='
> -        rest = dl.raw[eq + 1:]
> -        m = re.search(r'[<"]', rest)
> +        # First '<' or '"' after '=', but ignore comments and strip trailing
> +        # whitespace (e.g. remaining after removing the comment)
> +        rest = _strip_strings_and_comments(dl.raw[eq + 1:]).rstrip()
> +        m = re.search(r'\s*([<"])', rest)
>          if not m:
>              continue
> -        target_col = _display_col(dl.raw[:eq + 1 + m.start()])
> +        dl_value_complete = rest.endswith('",') or rest.endswith('>,')

[Severity: Medium]
Does this strict check correctly identify closed values in space-separated
arrays and concatenated strings?

In valid DTS, cell arrays or strings can be concatenated across lines without
commas:

    ranges = <...>
             <...>;

If a line ends with a closing bracket or double quote without a comma, the
block is closed, but dl_value_complete will incorrectly evaluate to False here.

> +        target_col = _display_col(_strip_strings_and_comments(dl.raw[:eq + 1 + m.start(1)]))

[Severity: Medium]
Will this calculate the correct column alignment if a block comment exists
between the = and the first opening bracket or double quote?

Because rest is populated using _strip_strings_and_comments(), any block
comments are removed, shortening the string. Using m.start(1) from this
stripped string as an index to slice the unstripped dl.raw discards the
visual width of those block comments, resulting in a target_col that is
too small.

>          for cont in dl.continuations:
> -            if _display_col(cont.indent_str) != target_col:
> +            target_offset = 0
> +            err_msg_explanation = 'to < or "'
> +            if not dl_value_complete:
> +                target_offset = 1
> +                err_msg_explanation = 'to the value under <'

[Severity: Medium]
Since dl_value_complete evaluates to False for comma-less concatenations as
noted above, doesn't this unconditionally apply target_offset = 1?

This would force developers to misalign their properly styled code by adding
an extra space, producing a false positive warning in the style checker.

> +            if _display_col(cont.indent_str) != target_col + target_offset:
>                  yield (cont.lineno,
>                         'continuation should align to column %d '
> -                       '(under < or ")' % (target_col + 1))
> +                       '(%s)' % (target_col + target_offset + 1, err_msg_explanation))
> +            # Align to the value within <> or "" of continuation (so the previous line)
> +            dl_value_complete = cont.stripped.endswith('",') or cont.stripped.endswith('>,')

[Severity: Medium]
This check appears to have the same flaw for updating the state on subsequent
continuation lines when valid comma-less arrays or strings are used.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826-n-dts-style-checker-continued-v5-0-fdd16ca6e771@oss.qualcomm.com?part=3