Re: [PATCH v3 1/3] dt-bindings: add DTS style checker

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The line classifier fails to handle trailing inline comments and whitespace, corrupting block depth and AST parsing.
- [High] The `mixed-indent-chars` rule directly conflicts with `continuation-alignment`, causing it to flag properly aligned continuation lines.
- [High] `check_continuation_alignment` incorrectly compares raw string length instead of visual display columns.
- [Medium] Regex patterns wrongly reject valid Devicetree identifiers starting with digits.
- [Medium] `check_value_whitespace` fails to validate cell array contents that span multiple lines.
--

commit fa5d5813b5bacc079c40709e0ab8be2c7e12de57
Author: Daniel Golle <daniel@makrotopia.org>

dt-bindings: add DTS style checker

This commit adds a Python tool that checks DTS coding style on examples in YAML
binding files and on .dts/.dtsi/.dtso source files. Rules are kept in a small
declarative registry and can be run in relaxed or strict modes.

> diff --git a/scripts/dtc/dt-check-style b/scripts/dtc/dt-check-style
> new file mode 100755
> index 0000000000000..e1dd548fe9486
> --- /dev/null
> +++ b/scripts/dtc/dt-check-style
> @@ -0,0 +1,1063 @@

[ ... ]

> +re_node_header = re.compile(
> +    r'^(?:([a-zA-Z_][a-zA-Z0-9_]*):\s*)?'
> +    r'([a-zA-Z][a-zA-Z0-9,._+-]*|/)'
> +    r'(?:@([0-9a-fA-F,]+))?'
> +    r'\s*\{$')

The Devicetree specification allows node names to begin with numeric digits.
Does this regex reject valid hardware nodes like 1wire@... or 3d-engine@... by
mandating that node names start with a letter?

[ ... ]

> +def parse_property_name(dl):
> +    m = re.match(r'^([a-zA-Z#][a-zA-Z0-9,._+#-]*)\s*[=;]', dl.stripped)
> +    if m:
> +        dl.prop_name = m.group(1)

Similarly, does this regex incorrectly reject property names that begin with
numeric digits by requiring them to start with a letter or #?

[ ... ]

> +def classify_lines(text):
> +    """Return a list of DtsLine. Tracks { } depth and groups
> +    continuation lines onto their leading PROPERTY line."""
> +    out = []
> +    in_block_comment = False
> +    prev_complete = True
> +    depth = 0
> +
> +    # Split preserving the indent string verbatim
> +    re_lead = re.compile(r'^([ \t]*)(.*)$')
> +
> +    for i, raw in enumerate(text.split('\n'), start=1):
> +        m = re_lead.match(raw)
> +        indent_str = m.group(1)
> +        stripped = m.group(2)

It appears the stripped variable retains trailing inline comments and
whitespace. Will lines formatted with trailing comments such as
node { /* comment */ or prop = <1>; // comment fail the subsequent suffix
checks like stripped.endswith('{') and stripped.endswith(';')?

If so, it seems node openings would be misclassified as properties, and
properties would erroneously set prev_complete = False, turning subsequent
lines into continuation lines.

[ ... ]

> +def check_mixed_indent_chars(ctx):
> +    """Indent must be all-spaces or all-tabs, never mixed on one line."""
> +    for dl in ctx.lines:
> +        if not dl.indent_str:
> +            continue
> +        if dl.linetype == LineType.PREPROCESSOR:
> +            continue
> +        if ' ' in dl.indent_str and '\t' in dl.indent_str:
> +            yield (dl.lineno, 'mixed tabs and spaces in indent')

Will this rule flag properly aligned continuation lines?

The standard practice for aligning multi-line properties in DTS is to use
tabs for base indentation and spaces for fine-grained alignment (e.g., to align
precisely under a < character). Unlike check_indent_consistent(), this rule
doesn't seem to exclude continuation lines.

[ ... ]

> +def check_value_whitespace(ctx):
> +    """Inside a <...> cell list use single spaces between values; no
> +    leading or trailing whitespace inside the brackets. Outside
> +    strings and comments only."""
> +    for dl in ctx.lines:
> +        if dl.linetype not in (LineType.PROPERTY, LineType.CONTINUATION):
> +            continue
> +        text = _strip_strings_and_comments(dl.raw)
> +        for m in re.finditer(r'<([^<>]*)>', text):

If a cell array declaration is split across multiple lines, will it bypass this
whitespace validation entirely?

Since this rule operates strictly on a line-by-line basis, neither line would
contain both < and > characters, which would cause the regex match to fail.

[ ... ]

> +def check_continuation_alignment(ctx):
> +    """A multi-line property's continuation lines must align their
> +    first non-whitespace character to the column of the first '<' or
> +    '"' after the '=' in the leading line."""
> +    for dl in ctx.lines:
> +        if dl.linetype != LineType.PROPERTY:
> +            continue
> +        if not dl.continuations:
> +            continue
> +        eq = dl.raw.find('=')
> +        if eq < 0:
> +            continue
> +        # First '<' or '"' after '='
> +        rest = dl.raw[eq + 1:]
> +        m = re.search(r'[<"]', rest)
> +        if not m:
> +            continue
> +        target_col = eq + 1 + m.start()
> +        for cont in dl.continuations:
> +            if len(cont.indent_str) != target_col:
> +                yield (cont.lineno,
> +                       'continuation should align to column %d '
> +                       '(under "<" or \\")' % (target_col + 1))

Does this logic accurately calculate display columns for lines using tab
indentation?

A properly aligned continuation line in a DTS file utilizing tabs will
naturally have a string length significantly shorter than target_col. This
checks len(cont.indent_str), which counts tab characters as length 1,
instead of calculating visual display width, which might result in false
positives for correctly aligned lines.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1778454442.git.daniel@makrotopia.org?part=1