From 94b63991459ab151ce33d525eaa13ae1f7d56f43 Mon Sep 17 00:00:00 2001 From: Tobias Reiher Date: Fri, 13 Sep 2024 17:57:41 +0200 Subject: [PATCH] Fix rejection of variable declarations with type Opaque Ref. eng/recordflux/RecordFlux#633 --- CHANGELOG.md | 1 + rflx/generator/state_machine.py | 44 +----- rflx/model/state_machine.py | 20 +++ tests/compilation/integration_test.py | 30 +++- tests/unit/generator/state_machine_test.py | 174 --------------------- tests/unit/graph_test.py | 6 +- tests/unit/model/state_machine_test.py | 51 +++++- 7 files changed, 100 insertions(+), 226 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9b6f55ff9..683f76ff8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Rejection of invalid parameter types and return types in function declarations (eng/recordflux/RecordFlux#977) - Consequential errors caused by undefined variables in binary expressions (eng/recordflux/RecordFlux#1672) +- Rejection of variable declarations with type `Opaque` (eng/recordflux/RecordFlux#633) ## [0.24.0] - 2024-09-12 diff --git a/rflx/generator/state_machine.py b/rflx/generator/state_machine.py index ac2857f0f..619c8b0c3 100644 --- a/rflx/generator/state_machine.py +++ b/rflx/generator/state_machine.py @@ -17,7 +17,6 @@ And, AndThen, Annotate, - Aspect, Assignment, Call, CallStatement, @@ -80,7 +79,6 @@ RecordType, Selected, Size, - SizeAspect, Slice, Statement, String, @@ -2918,7 +2916,7 @@ def always_true(_: ID) -> bool: is_global, declaration.location, declaration.expression, - state_machine_global=state_machine_global, + state_machine_global, ) if isinstance(declaration.type_, (ty.Message, ty.Sequence)): has_composite_declarations |= True @@ -3010,7 +3008,6 @@ def _declare( # noqa: PLR0912, PLR0913 is_global: Callable[[ID], bool], alloc_id: Location | None, expression: ir.ComplexExpr | None = None, - constant: bool = False, state_machine_global: bool = False, ) -> EvaluatedDeclaration: result = EvaluatedDeclaration() @@ -3021,44 +3018,7 @@ def _declare( # noqa: PLR0912, PLR0913 location=expression.expr.location, ) - if type_ == ty.OPAQUE: - initialization = None - object_type: Expr = Variable(const.TYPES_BYTES) - aspects: list[Aspect] = [] - - if expression: - if expression.is_expr() and isinstance(expression.expr, ir.Agg): - e = expression.expr - if len(e.elements) == 0: - object_type = Slice( - Variable(const.TYPES_BYTES), - Last(const.TYPES_INDEX), - First(const.TYPES_INDEX), - ) - initialization = None - if len(e.elements) > 0: - aspects.append( - SizeAspect(Mul(Number(len(e.elements)), Size(const.TYPES_BYTE))), - ) - initialization = expression.expr - else: - fail( - "initialization not yet supported", - location=expression.expr.location, - ) - - result.global_declarations.append( - ObjectDeclaration( - [identifier], - object_type, - self._to_ada_expr(initialization, is_global) if initialization else None, - constant=constant if initialization else False, - aliased=False, - aspects=aspects, - ), - ) - - elif isinstance(type_, (ty.UniversalInteger, ty.Integer, ty.Enumeration)): + if isinstance(type_, (ty.UniversalInteger, ty.Integer, ty.Enumeration)): result.global_declarations.append( ObjectDeclaration( [identifier], diff --git a/rflx/model/state_machine.py b/rflx/model/state_machine.py index 58ba31ccd..1f704646e 100644 --- a/rflx/model/state_machine.py +++ b/rflx/model/state_machine.py @@ -727,6 +727,26 @@ def undefined_type(type_identifier: StrID, location: Location | None) -> None: undefined_type(d.type_identifier, d.location) d.type_ = ty.Any() + if isinstance(d, decl.VariableDeclaration) and d.type_ == ty.OPAQUE: + assert d.type_identifier.location is not None + self.error.push( + ErrorEntry( + "invalid variable type", + Severity.ERROR, + d.type_identifier.location, + annotations=( + [ + Annotation( + "use a message with an opaque field instead", + Severity.HELP, + d.type_identifier.location, + ), + ] + ), + generate_default_annotation=False, + ), + ) + if isinstance(d, decl.FunctionDeclaration): for p in d.parameters: parameter_id = type_decl.internal_type_identifier( diff --git a/tests/compilation/integration_test.py b/tests/compilation/integration_test.py index 5be2d9c81..b086c0ca9 100644 --- a/tests/compilation/integration_test.py +++ b/tests/compilation/integration_test.py @@ -930,17 +930,37 @@ def test_state_machine_message_field_access_in_transition(tmp_path: Path) -> Non @pytest.mark.parametrize( - ("global_decl", "local_decl", "value"), + ("global_decl", "local_decl", "value", "error"), [ - ("Key : Opaque := [0, 1, 0];", "", "Key"), - ("", "Key : Opaque := [0, 1, 0];", "Key"), - ("", "", "[0, 1, 0]"), + ( + "Key : Opaque := [0, 1, 0];", + "", + "Key", + r":13:13: error: invalid variable type\n" + r":13:13: help: use a message with an opaque field instead\n" + r":22:17: error: comparisons of opaque fields not yet supported", + ), + ( + "", + "Key : Opaque := [0, 1, 0];", + "Key", + r":18:15: error: invalid variable type\n" + r":18:15: help: use a message with an opaque field instead\n" + r":22:17: error: comparisons of opaque fields not yet supported", + ), + ( + "", + "", + "[0, 1, 0]", + r":22:17: error: comparisons of opaque fields not yet supported", + ), ], ) def test_state_machine_comparing_opaque_values_in_comprehension( global_decl: str, local_decl: str, value: str, + error: str, tmp_path: Path, ) -> None: spec = f"""\ @@ -980,6 +1000,6 @@ def test_state_machine_comparing_opaque_values_in_comprehension( # TODO(eng/recordflux/RecordFlux#1497): Support comparisons of opaque fields with pytest.raises( RecordFluxError, - match=r"^:22:17: error: comparisons of opaque fields not yet supported$", + match=rf"^{error}$", ): utils.assert_compilable_code_string(spec, tmp_path) diff --git a/tests/unit/generator/state_machine_test.py b/tests/unit/generator/state_machine_test.py index 3c631ce59..c8edc6bad 100644 --- a/tests/unit/generator/state_machine_test.py +++ b/tests/unit/generator/state_machine_test.py @@ -1,7 +1,6 @@ from __future__ import annotations import typing -from dataclasses import dataclass from functools import lru_cache from typing import Callable, Sequence @@ -482,166 +481,6 @@ def test_state_machine_evaluate_declarations( ) -@dataclass -class EvaluatedDeclarationStr: - global_declarations: str = "" - initialization_declarations: str = "" - initialization: str = "" - finalization: str = "" - - -@pytest.mark.parametrize( - ("type_", "expression", "constant", "state_machine_global", "expected"), - [ - ( - ty.OPAQUE, - ir.ComplexExpr([], ir.Agg([])), - False, - False, - EvaluatedDeclarationStr( - global_declarations=( - "X : RFLX_Types.Bytes (RFLX_Types.Index'Last .. RFLX_Types.Index'First);" - ), - ), - ), - ( - ty.OPAQUE, - ir.ComplexExpr([], ir.Agg([])), - True, - False, - EvaluatedDeclarationStr( - global_declarations=( - "X : RFLX_Types.Bytes (RFLX_Types.Index'Last .. RFLX_Types.Index'First);" - ), - ), - ), - ( - ty.OPAQUE, - ir.ComplexExpr([], ir.Agg([ir.IntVal(1)])), - False, - False, - EvaluatedDeclarationStr( - global_declarations=( - "X : RFLX_Types.Bytes := (RFLX_Types.Index'First => RFLX_Types.Byte'Val (1))" - " with\n Size =>\n 1 * RFLX_Types.Byte'Size;" - ), - ), - ), - ( - ty.OPAQUE, - ir.ComplexExpr([], ir.Agg([ir.IntVal(1)])), - True, - False, - EvaluatedDeclarationStr( - global_declarations=( - "X : constant RFLX_Types.Bytes :=" - " (RFLX_Types.Index'First => RFLX_Types.Byte'Val (1))" - " with\n Size =>\n 1 * RFLX_Types.Byte'Size;" - ), - ), - ), - ( - ty.OPAQUE, - ir.ComplexExpr([], ir.Agg([ir.IntVal(1), ir.IntVal(2)])), - False, - False, - EvaluatedDeclarationStr( - global_declarations=( - "X : RFLX_Types.Bytes := (RFLX_Types.Byte'Val (1), RFLX_Types.Byte'Val (2))" - " with\n Size =>\n 2 * RFLX_Types.Byte'Size;" - ), - ), - ), - ( - ty.OPAQUE, - ir.ComplexExpr([], ir.Agg([ir.IntVal(1), ir.IntVal(2)])), - False, - True, - EvaluatedDeclarationStr( - global_declarations=( - "X : RFLX_Types.Bytes := (RFLX_Types.Byte'Val (1), RFLX_Types.Byte'Val (2))" - " with\n Size =>\n 2 * RFLX_Types.Byte'Size;" - ), - ), - ), - ( - ty.OPAQUE, - ir.ComplexExpr([], ir.Agg([ir.IntVal(1), ir.IntVal(2)])), - True, - False, - EvaluatedDeclarationStr( - global_declarations=( - "X : constant RFLX_Types.Bytes :=" - " (RFLX_Types.Byte'Val (1), RFLX_Types.Byte'Val (2))" - " with\n Size =>\n 2 * RFLX_Types.Byte'Size;" - ), - ), - ), - ( - ty.OPAQUE, - None, - False, - False, - EvaluatedDeclarationStr( - global_declarations=("X : RFLX_Types.Bytes;"), - ), - ), - ( - ty.OPAQUE, - None, - False, - True, - EvaluatedDeclarationStr( - global_declarations=("X : RFLX_Types.Bytes;"), - ), - ), - ( - ty.OPAQUE, - None, - True, - False, - EvaluatedDeclarationStr( - global_declarations=("X : RFLX_Types.Bytes;"), - ), - ), - ], -) -def test_state_machine_declare( - type_: ty.Type, - expression: ir.ComplexExpr | None, - constant: bool, - state_machine_global: bool, - expected: EvaluatedDeclarationStr, -) -> None: - loc: Location = Location((1, 1)) - allocator = AllocatorGenerator(dummy_state_machine(), Integration()) - - allocator._allocation_slots[loc] = 1 # noqa: SLF001 - fsm_generator = FSMGenerator( - dummy_state_machine(), - Integration(), - allocator, - debug=Debug.BUILTIN, - ) - - result = fsm_generator._declare( # noqa: SLF001 - ID("X"), - type_, - lambda _: False, - loc, - expression, - constant, - state_machine_global, - ) - assert "\n".join(str(d) for d in result.global_declarations) == expected.global_declarations - assert ( - "\n".join(str(d) for d in result.initialization_declarations) - == expected.initialization_declarations - ) - assert "\n".join(str(s) for s in result.initialization) == expected.initialization - assert "\n".join(str(s) for s in result.finalization) == expected.finalization - - @pytest.mark.parametrize( ("type_", "expression", "error_type", "error_msg"), [ @@ -660,19 +499,6 @@ def test_state_machine_declare( RecordFluxError, r"initialization using function call not yet supported", ), - ( - ty.OPAQUE, - ir.ComplexExpr( - [ir.Assign("X", ir.IntVal(0), INT_TY)], - ir.IntVar( - "X", - INT_TY, - origin=ir.ConstructedOrigin("X", Location((10, 20))), - ), - ), - RecordFluxError, - r"initialization not yet supported", - ), ( ty.Message("T"), ir.ComplexExpr( diff --git a/tests/unit/graph_test.py b/tests/unit/graph_test.py index 75efef92f..e735f7d98 100644 --- a/tests/unit/graph_test.py +++ b/tests/unit/graph_test.py @@ -10,7 +10,6 @@ BOOLEAN, FINAL, INITIAL, - OPAQUE, Field, Integer, Link, @@ -22,6 +21,7 @@ statement as stmt, ) from rflx.rapidflux import Location, RecordFluxError +from tests.data import models def assert_graph(graph: Dot, expected: str, tmp_path: Path) -> None: @@ -222,7 +222,7 @@ def test_state_machine_graph(tmp_path: Path) -> None: "STATE", transitions=[Transition(target=ID("IGNORED_1")), Transition(target=ID("null"))], actions=[stmt.VariableAssignment("Global", FALSE), stmt.Reset("Local")], - declarations=[decl.VariableDeclaration("Local", "Opaque")], + declarations=[decl.VariableDeclaration("Local", "TLV::Message")], ), State( "IGNORED_1", @@ -231,7 +231,7 @@ def test_state_machine_graph(tmp_path: Path) -> None: ], declarations=[decl.VariableDeclaration("Global", "Boolean")], parameters=[], - types=[BOOLEAN, OPAQUE], + types=[BOOLEAN, models.tlv_message()], ) expected_full = r""" diff --git a/tests/unit/model/state_machine_test.py b/tests/unit/model/state_machine_test.py index c1b7f8fe7..736eec760 100644 --- a/tests/unit/model/state_machine_test.py +++ b/tests/unit/model/state_machine_test.py @@ -728,6 +728,47 @@ def test_declared_local_variable_message_field() -> None: ) +def test_invalid_variable_type() -> None: + assert_state_machine_model_error( + states=[ + State( + "Start", + transitions=[ + Transition( + target=ID("null"), + ), + ], + declarations=[ + decl.VariableDeclaration( + "Y", + ID("Opaque", Location((2, 5))), + location=Location((2, 3)), + ), + ], + ), + ], + declarations=[ + decl.VariableDeclaration( + "X", + ID("Opaque", Location((1, 4))), + location=Location((1, 2)), + ), + ], + parameters=[], + types=[OPAQUE], + regex=( + r"^" + r":1:4: error: invalid variable type\n" + r":1:4: help: use a message with an opaque field instead\n" + r":2:5: error: invalid variable type\n" + r":2:5: help: use a message with an opaque field instead\n" + r':1:2: error: unused variable "X"\n' + r':2:3: error: unused variable "Y"' + r"$" + ), + ) + + def test_assignment_to_undeclared_variable() -> None: assert_state_machine_model_error( states=[ @@ -1956,12 +1997,18 @@ def test_conversion_invalid_argument() -> None: ), ], declarations=[ - decl.VariableDeclaration("Message", "Opaque"), + decl.VariableDeclaration("Message", ID("Opaque", Location((1, 2)))), decl.VariableDeclaration("Converted", "TLV::Message"), ], parameters=[], types=[OPAQUE, models.tlv_message()], - regex=(r"^:10:20: error: invalid argument for conversion, expected message field$"), + regex=( + r"^" + r":1:2: error: invalid variable type\n" + r":1:2: help: use a message with an opaque field instead\n" + r":10:20: error: invalid argument for conversion, expected message field" + r"$" + ), )