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

Add matterlint support for deny cluster .... attribute ... #30147

Merged
merged 10 commits into from
Nov 2, 2023
3 changes: 0 additions & 3 deletions .github/workflows/lint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,6 @@ jobs:
# https://github.com/project-chip/connectedhomeip/issues/19176
# https://github.com/project-chip/connectedhomeip/issues/19175
# https://github.com/project-chip/connectedhomeip/issues/19173
# https://github.com/project-chip/connectedhomeip/issues/19169
# https://github.com/project-chip/connectedhomeip/issues/22640
if [ "$idl_file" = './examples/all-clusters-app/all-clusters-common/all-clusters-app.matter' ]; then continue; fi
if [ "$idl_file" = './examples/log-source-app/log-source-common/log-source-app.matter' ]; then continue; fi
if [ "$idl_file" = './examples/placeholder/linux/apps/app1/config.matter' ]; then continue; fi
if [ "$idl_file" = './examples/placeholder/linux/apps/app2/config.matter' ]; then continue; fi
Expand Down
18 changes: 13 additions & 5 deletions scripts/py_matter_idl/matter_idl/lint/lint_rules_grammar.lark
Original file line number Diff line number Diff line change
Expand Up @@ -4,25 +4,33 @@ instruction: load_xml|all_endpoint_rule|specific_endpoint_rule

load_xml: "load" ESCAPED_STRING ";"

all_endpoint_rule: "all" "endpoints" "{" required_global_attribute* "}"
all_endpoint_rule: "all" "endpoints" "{" (required_global_attribute|denylist_cluster_attribute)* "}"

specific_endpoint_rule: "endpoint" integer "{" (required_server_cluster|rejected_server_cluster)* "}"

required_global_attribute: "require" "global" "attribute" id "=" integer ";"

required_server_cluster: "require" "server" "cluster" (id|POSITIVE_INTEGER|HEX_INTEGER) ";"
required_server_cluster: "require" "server" "cluster" id_or_number ";"

rejected_server_cluster: "reject" "server" "cluster" (id|POSITIVE_INTEGER|HEX_INTEGER) ";"
rejected_server_cluster: "reject" "server" "cluster" id_or_number ";"

// Allows deny like:
andy31415 marked this conversation as resolved.
Show resolved Hide resolved
// deny cluster 234; // Deny an entire cluster
// deny cluster DescriptorCluster attribute 123; // attribute deny (mix name and number)
// deny cluster 11 attribute 22; // attribute deny
denylist_cluster_attribute: "deny" "cluster" id_or_number ["attribute" id_or_number] ";"

integer: positive_integer | negative_integer

?id_or_number: (id|positive_integer)

positive_integer: POSITIVE_INTEGER | HEX_INTEGER
negative_integer: "-" positive_integer

id: ID

POSITIVE_INTEGER: /\d+/
HEX_INTEGER: /0x[A-Fa-f0-9]+/
POSITIVE_INTEGER: /\d+/
HEX_INTEGER: /0x[A-Fa-f0-9]+/
ID: /[a-zA-Z_][a-zA-Z0-9_]*/

%import common.ESCAPED_STRING
Expand Down
28 changes: 20 additions & 8 deletions scripts/py_matter_idl/matter_idl/lint/lint_rules_parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,15 @@
from lark.visitors import Discard, Transformer, v_args

try:
from .types import (AttributeRequirement, ClusterCommandRequirement, ClusterRequirement, ClusterValidationRule,
RequiredAttributesRule, RequiredCommandsRule)
from .types import (AttributeRequirement, ClusterAttributeDeny, ClusterCommandRequirement, ClusterRequirement,
ClusterValidationRule, RequiredAttributesRule, RequiredCommandsRule)
except ImportError:
import sys

sys.path.append(os.path.join(os.path.abspath(
os.path.dirname(__file__)), "..", ".."))
from matter_idl.lint.types import (AttributeRequirement, ClusterCommandRequirement, ClusterRequirement, ClusterValidationRule,
RequiredAttributesRule, RequiredCommandsRule)
from matter_idl.lint.types import (AttributeRequirement, ClusterAttributeDeny, ClusterCommandRequirement, ClusterRequirement,
ClusterValidationRule, RequiredAttributesRule, RequiredCommandsRule)


class ElementNotFoundError(Exception):
Expand Down Expand Up @@ -167,6 +167,9 @@ def GetLinterRules(self):
def RequireAttribute(self, r: AttributeRequirement):
self._required_attributes_rule.RequireAttribute(r)

def Deny(self, what: ClusterAttributeDeny):
self._required_attributes_rule.Deny(what)

def FindClusterCode(self, name: str) -> Optional[Tuple[str, int]]:
if name not in self._cluster_codes:
# Name may be a number. If this can be parsed as a number, accept it anyway
Expand Down Expand Up @@ -281,9 +284,14 @@ def start(self, instructions):
def instruction(self, instruction):
return Discard

def all_endpoint_rule(self, attributes):
for attribute in attributes:
self.context.RequireAttribute(attribute)
def all_endpoint_rule(self, rules: List[Union[AttributeRequirement, ClusterAttributeDeny]]):
for rule in rules:
if type(rule) is AttributeRequirement:
self.context.RequireAttribute(rule)
elif type(rule) is ClusterAttributeDeny:
self.context.Deny(rule)
else:
raise Exception("Unkown endpoint requirement: %r" % rule)

return Discard

Expand Down Expand Up @@ -320,6 +328,10 @@ def required_server_cluster(self, id):
def rejected_server_cluster(self, id):
return ServerClusterRequirement(ClusterActionEnum.REJECT, id)

@v_args(inline=True)
def denylist_cluster_attribute(self, cluster_id, attribute_id):
return ClusterAttributeDeny(cluster_id, attribute_id)


class Parser:
def __init__(self, parser, file_name: str):
Expand All @@ -337,7 +349,7 @@ def CreateParser(file_name: str):
Generates a parser that will process a ".matter" file into a IDL
"""
return Parser(
Lark.open('lint_rules_grammar.lark', rel_to=__file__, parser='lalr', propagate_positions=True), file_name=file_name)
Lark.open('lint_rules_grammar.lark', rel_to=__file__, parser='lalr', propagate_positions=True, maybe_placeholders=True), file_name=file_name)


if __name__ == '__main__':
Expand Down
41 changes: 39 additions & 2 deletions scripts/py_matter_idl/matter_idl/lint/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

from abc import ABC, abstractmethod
from dataclasses import dataclass, field
from typing import List, MutableMapping, Optional
from typing import List, MutableMapping, Optional, Union

from matter_idl.matter_idl_types import ClusterSide, Idl, ParseMetaData

Expand Down Expand Up @@ -75,6 +75,12 @@ class AttributeRequirement:
filter_cluster: Optional[int] = field(default=None)


@dataclass
class ClusterAttributeDeny:
cluster_id: Union[str, int]
attribute_id: Union[str, int]


@dataclass
class ClusterRequirement:
endpoint_id: int
Expand Down Expand Up @@ -202,6 +208,7 @@ class RequiredAttributesRule(ErrorAccumulatingRule):
def __init__(self, name):
super().__init__(name)
self._mandatory_attributes: List[AttributeRequirement] = []
self._deny_attributes: List[ClusterAttributeDeny] = []

def __repr__(self):
result = "RequiredAttributesRule{\n"
Expand All @@ -211,13 +218,22 @@ def __repr__(self):
for attr in self._mandatory_attributes:
result += " - %r\n" % attr

if self._deny_attributes:
result += " deny_attributes:\n"
for attr in self._deny_attributes:
result += " - %r\n" % attr

result += "}"
return result

def RequireAttribute(self, attr: AttributeRequirement):
"""Mark an attribute required"""
self._mandatory_attributes.append(attr)

def Deny(self, what: ClusterAttributeDeny):
"""Mark a cluster (or cluster/attribute) as denied"""
self._deny_attributes.append(what)

def _ServerClusterDefinition(self, name: str, location: Optional[LocationInFile]):
"""Finds the server cluster definition with the given name.

Expand Down Expand Up @@ -275,7 +291,7 @@ def _LintImpl(self):

attribute_codes.add(name_to_code_map[attr.name])

# Linting codes now
# Linting required attributes
for check in self._mandatory_attributes:
if check.filter_cluster is not None and check.filter_cluster != cluster_definition.code:
continue
Expand All @@ -286,6 +302,27 @@ def _LintImpl(self):
check.name, check.code),
self._ParseLocation(cluster.parse_meta))

# Lint rejected attributes
for check in self._deny_attributes:
if check.cluster_id not in [cluster_definition.name, cluster_definition.code]:
continue # different cluster

if check.attribute_id is None:
self._AddLintError(
f"EP{endpoint.number}: cluster {cluster_definition.name}({cluster_definition.code}) is DENIED!",
self._ParseLocation(cluster.parse_meta))
continue

# figure out every attribute that may be denied
# We already know every attribute name and have codes
for attr in cluster.attributes:
if check.attribute_id in [attr.name, name_to_code_map[attr.name]]:
cluster_str = f"{cluster_definition.name}({cluster_definition.code})"
attribute_str = f"{attr.name}({name_to_code_map[attr.name]})"
self._AddLintError(
f"EP{endpoint.number}: attribute {cluster_str}::{attribute_str} is DENIED!",
self._ParseLocation(cluster.parse_meta))


@dataclass
class ClusterCommandRequirement:
Expand Down
10 changes: 10 additions & 0 deletions scripts/rules.matterlint
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,16 @@ all endpoints {

require global attribute featureMap = 65532;
require global attribute clusterRevision = 65533;

// Deny rules:
// deny cluster Foo; // denies entire cluster by name
// deny cluster 123; // denies entire cluster by id
// deny cluster Foo attribute bar; // denies specific attribute by name
andy31415 marked this conversation as resolved.
Show resolved Hide resolved
// deny cluster Foo attribute 123; // denies specific attribute by id
// deny cluster 234 attribute 567; // Allows deny using ids only

// (previously AverageWearCount, see #30002/#29285 in GitHub)
deny cluster GeneralDiagnostics attribute 9;
}

endpoint 0 {
Expand Down