Copilot commented on code in PR #3528: URL: https://github.com/apache/fory/pull/3528#discussion_r3006062110
########## compiler/fory_compiler/tests/test_ir_service_validation.py: ########## @@ -0,0 +1,308 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +"""Tests for IR-level service validation rules.""" + +from fory_compiler.frontend.fdl.parser import Parser +from fory_compiler.frontend.proto.lexer import Lexer as ProtoLexer +from fory_compiler.frontend.proto.parser import Parser as ProtoParser +from fory_compiler.frontend.proto.translator import ProtoTranslator +from fory_compiler.frontend.fbs.lexer import Lexer as FbsLexer +from fory_compiler.frontend.fbs.parser import Parser as FbsParser +from fory_compiler.frontend.fbs.translator import FbsTranslator +from fory_compiler.ir.validator import SchemaValidator + + +def parse_fdl(source: str): + return Parser.from_source(source).parse() + + +def parse_proto(source: str): + lexer = ProtoLexer(source) + parser = ProtoParser(lexer.tokenize()) + return ProtoTranslator(parser.parse()).translate() + + +def parse_fbs(source: str): + lexer = FbsLexer(source) + parser = FbsParser(lexer.tokenize()) + return FbsTranslator(parser.parse()).translate() + + +def validate(schema): + validator = SchemaValidator(schema) + validator.validate() + return validator + + +def test_duplicate_service_names_fails_validation(): + source = """ + package test; + + service Alpha {} + service Alpha {} + """ + v = validate(parse_fdl(source)) + assert any("Duplicate service name: Alpha" in e.message for e in v.errors) + + +def test_unique_service_names_passes_validation(): + source = """ + package test; + + service Alpha {} + service Beta {} + """ + v = validate(parse_fdl(source)) + assert v.errors == [] + + +def test_duplicate_method_names_fails_validation(): + source = """ + package test; + + message Req {} + message Res {} + + service Greeter { + rpc SayHello (Req) returns (Res); + rpc SayHello (Req) returns (Res); + } + """ + v = validate(parse_fdl(source)) + assert any( + "Duplicate method name in service Greeter: SayHello" in e.message + for e in v.errors + ) + + +def test_same_method_name_in_different_services_passes_validation(): + source = """ + package test; + + message Req {} + message Res {} + + service Alpha { + rpc SayHello (Req) returns (Res); + } + + service Beta { + rpc SayHello (Req) returns (Res); + } + """ + v = validate(parse_fdl(source)) + assert v.errors == [] + + +def test_unique_method_names_passes_validation(): + source = """ + package test; + + message Req {} + message Res {} + + service Greeter { + rpc SayHello (Req) returns (Res); + rpc SayGoodbye (Req) returns (Res); + } + """ + v = validate(parse_fdl(source)) + assert v.errors == [] + + +def test_rpc_request_type_enum_fails_validation(): + source = """ + package test; + + enum Status { OK = 0; } + message Res {} + + service Svc { + rpc Call (Status) returns (Res); + } + """ + v = validate(parse_fdl(source)) + assert any( + "RPC type 'Status'" in e.message and "not a enum" in e.message + for e in v.errors + ) + + +def test_rpc_response_type_enum_fails_validation(): + source = """ + package test; + + message Req {} + enum Status { OK = 0; } + + service Svc { + rpc Call (Req) returns (Status); + } + """ + v = validate(parse_fdl(source)) + assert any( + "RPC type 'Status'" in e.message and "not a enum" in e.message + for e in v.errors + ) + + +def test_rpc_request_type_union_fails_validation(): + source = """ + package test; + + union Payload { string text = 1; } + message Res {} + + service Svc { + rpc Call (Payload) returns (Res); + } + """ + v = validate(parse_fdl(source)) + assert any( + "RPC type 'Payload'" in e.message and "not a union" in e.message + for e in v.errors + ) Review Comment: This assertion keys off the exact wording "not a union". To keep the test resilient to small message wording changes (while still validating behavior), consider asserting on stable fragments like "must be a message" and "union" instead of matching the entire phrase. ########## compiler/fory_compiler/tests/test_ir_service_validation.py: ########## @@ -0,0 +1,308 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +"""Tests for IR-level service validation rules.""" + +from fory_compiler.frontend.fdl.parser import Parser +from fory_compiler.frontend.proto.lexer import Lexer as ProtoLexer +from fory_compiler.frontend.proto.parser import Parser as ProtoParser +from fory_compiler.frontend.proto.translator import ProtoTranslator +from fory_compiler.frontend.fbs.lexer import Lexer as FbsLexer +from fory_compiler.frontend.fbs.parser import Parser as FbsParser +from fory_compiler.frontend.fbs.translator import FbsTranslator +from fory_compiler.ir.validator import SchemaValidator + + +def parse_fdl(source: str): + return Parser.from_source(source).parse() + + +def parse_proto(source: str): + lexer = ProtoLexer(source) + parser = ProtoParser(lexer.tokenize()) + return ProtoTranslator(parser.parse()).translate() + + +def parse_fbs(source: str): + lexer = FbsLexer(source) + parser = FbsParser(lexer.tokenize()) + return FbsTranslator(parser.parse()).translate() + + +def validate(schema): + validator = SchemaValidator(schema) + validator.validate() + return validator + + +def test_duplicate_service_names_fails_validation(): + source = """ + package test; + + service Alpha {} + service Alpha {} + """ + v = validate(parse_fdl(source)) + assert any("Duplicate service name: Alpha" in e.message for e in v.errors) + + +def test_unique_service_names_passes_validation(): + source = """ + package test; + + service Alpha {} + service Beta {} + """ + v = validate(parse_fdl(source)) + assert v.errors == [] + + +def test_duplicate_method_names_fails_validation(): + source = """ + package test; + + message Req {} + message Res {} + + service Greeter { + rpc SayHello (Req) returns (Res); + rpc SayHello (Req) returns (Res); + } + """ + v = validate(parse_fdl(source)) + assert any( + "Duplicate method name in service Greeter: SayHello" in e.message + for e in v.errors + ) + + +def test_same_method_name_in_different_services_passes_validation(): + source = """ + package test; + + message Req {} + message Res {} + + service Alpha { + rpc SayHello (Req) returns (Res); + } + + service Beta { + rpc SayHello (Req) returns (Res); + } + """ + v = validate(parse_fdl(source)) + assert v.errors == [] + + +def test_unique_method_names_passes_validation(): + source = """ + package test; + + message Req {} + message Res {} + + service Greeter { + rpc SayHello (Req) returns (Res); + rpc SayGoodbye (Req) returns (Res); + } + """ + v = validate(parse_fdl(source)) + assert v.errors == [] + + +def test_rpc_request_type_enum_fails_validation(): + source = """ + package test; + + enum Status { OK = 0; } + message Res {} + + service Svc { + rpc Call (Status) returns (Res); + } + """ + v = validate(parse_fdl(source)) + assert any( + "RPC type 'Status'" in e.message and "not a enum" in e.message + for e in v.errors + ) + + +def test_rpc_response_type_enum_fails_validation(): + source = """ + package test; + + message Req {} + enum Status { OK = 0; } + + service Svc { + rpc Call (Req) returns (Status); + } + """ + v = validate(parse_fdl(source)) + assert any( + "RPC type 'Status'" in e.message and "not a enum" in e.message + for e in v.errors + ) Review Comment: This assertion depends on the exact phrase "not a enum". If the validator message is updated to correct grammar ("not an enum") or otherwise tweaked, this will fail despite correct behavior. Prefer checking for stable substrings like "must be a message" and "enum". ########## compiler/fory_compiler/ir/validator.py: ########## @@ -639,6 +640,37 @@ def check_message_fields( for f in union.fields: check_field(f, None) + def _check_services(self) -> None: + seen_service_names: dict = {} + for service in self.schema.services: + if service.name in seen_service_names: + self._error( + f"Duplicate service name: {service.name}", + service.location, + ) + seen_service_names.setdefault(service.name, service.location) + + seen_method_names: dict = {} + for method in service.methods: + if method.name in seen_method_names: + self._error( + f"Duplicate method name in service {service.name}: {method.name}", + method.location, + ) + seen_method_names.setdefault(method.name, method.location) + + for named_type in (method.request_type, method.response_type): + resolved = self.schema.get_type(named_type.name) + if resolved is None: + continue + if not isinstance(resolved, Message): + kind = "enum" if isinstance(resolved, Enum) else "union" + self._error( + f"RPC type '{named_type.name}' in service {service.name}" + f" must be a message, not a {kind}", Review Comment: The error text uses “not a enum” when `kind == "enum"`. Since this is user-facing validation output, it should be grammatically correct (e.g., “not an enum” while keeping “not a union” unchanged). This will also require updating the corresponding test assertions that currently match the incorrect phrasing. ```suggestion if isinstance(resolved, Enum): kind = "enum" article = "an" else: kind = "union" article = "a" self._error( f"RPC type '{named_type.name}' in service {service.name}" f" must be a message, not {article} {kind}", ``` ########## compiler/fory_compiler/tests/test_service_codegen.py: ########## @@ -0,0 +1,144 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +"""Codegen smoke tests for schemas that contain service definitions.""" + +from pathlib import Path +from textwrap import dedent +from typing import Dict, Tuple, Type + +from fory_compiler.cli import compile_file +from fory_compiler.frontend.fdl.lexer import Lexer +from fory_compiler.frontend.fdl.parser import Parser +from fory_compiler.generators.base import BaseGenerator, GeneratorOptions +from fory_compiler.generators.cpp import CppGenerator +from fory_compiler.generators.csharp import CSharpGenerator +from fory_compiler.generators.go import GoGenerator +from fory_compiler.generators.java import JavaGenerator +from fory_compiler.generators.python import PythonGenerator +from fory_compiler.generators.rust import RustGenerator +from fory_compiler.generators.swift import SwiftGenerator +from fory_compiler.ir.ast import Schema + + +GENERATOR_CLASSES: Tuple[Type[BaseGenerator], ...] = ( + JavaGenerator, + PythonGenerator, + CppGenerator, + RustGenerator, + GoGenerator, + CSharpGenerator, + SwiftGenerator, +) + +_GREETER_WITH_SERVICE = dedent( + """ + package demo.greeter; + + message HelloRequest { + string name = 1; + } + + message HelloReply { + string reply = 1; + } + + service Greeter { + rpc SayHello (HelloRequest) returns (HelloReply); + } + """ +) + +_GREETER_WITHOUT_SERVICE = dedent( + """ + package demo.greeter; + + message HelloRequest { + string name = 1; + } + + message HelloReply { + string reply = 1; + } + """ +) + + +def parse_fdl(source: str) -> Schema: + return Parser(Lexer(source).tokenize()).parse() + + +def generate_files(schema: Schema, generator_cls: Type[BaseGenerator]) -> Dict[str, str]: + options = GeneratorOptions(output_dir=Path("/tmp")) + generator = generator_cls(schema, options) + return {item.path: item.content for item in generator.generate()} + + +def test_service_definition_does_not_affect_message_codegen(): + schema_with = parse_fdl(_GREETER_WITH_SERVICE) + schema_without = parse_fdl(_GREETER_WITHOUT_SERVICE) + for generator_cls in GENERATOR_CLASSES: + files_with = generate_files(schema_with, generator_cls) + files_without = generate_files(schema_without, generator_cls) + assert files_with == files_without, ( + f"{generator_cls.language_name}: service definition changed message output" + ) + + +def test_generate_services_returns_empty_list_for_all_generators(): + schema = parse_fdl(_GREETER_WITH_SERVICE) + for generator_cls in GENERATOR_CLASSES: + options = GeneratorOptions(output_dir=Path("/tmp")) + generator = generator_cls(schema, options) + assert generator.generate_services() == [], ( + f"{generator_cls.language_name}: generate_services() should return [] until gRPC is implemented" + ) + + +def test_service_schema_produces_one_file_per_message_per_language(): + schema = parse_fdl(_GREETER_WITH_SERVICE) + for generator_cls in GENERATOR_CLASSES: + files = generate_files(schema, generator_cls) + assert len(files) >= 1, ( + f"{generator_cls.language_name}: expected at least one generated file" Review Comment: The test name says “one file per message per language”, but the assertion only checks `len(files) >= 1`. Either tighten the assertion to match the stated intent (if that invariant is actually true for all generators) or rename the test to reflect what it really validates (e.g., that codegen produces at least one file when services are present). ```suggestion def test_service_schema_produces_files_for_all_generators(): schema = parse_fdl(_GREETER_WITH_SERVICE) for generator_cls in GENERATOR_CLASSES: files = generate_files(schema, generator_cls) assert len(files) >= 1, ( f"{generator_cls.language_name}: expected at least one generated file for service schema" ``` ########## compiler/fory_compiler/tests/test_ir_service_validation.py: ########## @@ -0,0 +1,308 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +"""Tests for IR-level service validation rules.""" + +from fory_compiler.frontend.fdl.parser import Parser +from fory_compiler.frontend.proto.lexer import Lexer as ProtoLexer +from fory_compiler.frontend.proto.parser import Parser as ProtoParser +from fory_compiler.frontend.proto.translator import ProtoTranslator +from fory_compiler.frontend.fbs.lexer import Lexer as FbsLexer +from fory_compiler.frontend.fbs.parser import Parser as FbsParser +from fory_compiler.frontend.fbs.translator import FbsTranslator +from fory_compiler.ir.validator import SchemaValidator + + +def parse_fdl(source: str): + return Parser.from_source(source).parse() + + +def parse_proto(source: str): + lexer = ProtoLexer(source) + parser = ProtoParser(lexer.tokenize()) + return ProtoTranslator(parser.parse()).translate() + + +def parse_fbs(source: str): + lexer = FbsLexer(source) + parser = FbsParser(lexer.tokenize()) + return FbsTranslator(parser.parse()).translate() + + +def validate(schema): + validator = SchemaValidator(schema) + validator.validate() + return validator + + +def test_duplicate_service_names_fails_validation(): + source = """ + package test; + + service Alpha {} + service Alpha {} + """ + v = validate(parse_fdl(source)) + assert any("Duplicate service name: Alpha" in e.message for e in v.errors) + + +def test_unique_service_names_passes_validation(): + source = """ + package test; + + service Alpha {} + service Beta {} + """ + v = validate(parse_fdl(source)) + assert v.errors == [] + + +def test_duplicate_method_names_fails_validation(): + source = """ + package test; + + message Req {} + message Res {} + + service Greeter { + rpc SayHello (Req) returns (Res); + rpc SayHello (Req) returns (Res); + } + """ + v = validate(parse_fdl(source)) + assert any( + "Duplicate method name in service Greeter: SayHello" in e.message + for e in v.errors + ) + + +def test_same_method_name_in_different_services_passes_validation(): + source = """ + package test; + + message Req {} + message Res {} + + service Alpha { + rpc SayHello (Req) returns (Res); + } + + service Beta { + rpc SayHello (Req) returns (Res); + } + """ + v = validate(parse_fdl(source)) + assert v.errors == [] + + +def test_unique_method_names_passes_validation(): + source = """ + package test; + + message Req {} + message Res {} + + service Greeter { + rpc SayHello (Req) returns (Res); + rpc SayGoodbye (Req) returns (Res); + } + """ + v = validate(parse_fdl(source)) + assert v.errors == [] + + +def test_rpc_request_type_enum_fails_validation(): + source = """ + package test; + + enum Status { OK = 0; } + message Res {} + + service Svc { + rpc Call (Status) returns (Res); + } + """ + v = validate(parse_fdl(source)) + assert any( + "RPC type 'Status'" in e.message and "not a enum" in e.message + for e in v.errors + ) Review Comment: This assertion matches the exact phrase "not a enum", which is both ungrammatical and makes the test brittle to minor error-message wording changes. Consider asserting on more stable substrings (e.g., "must be a message" + "enum") so the validator message can be corrected without breaking the test. ########## compiler/fory_compiler/tests/test_ir_service_validation.py: ########## @@ -0,0 +1,308 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +"""Tests for IR-level service validation rules.""" + +from fory_compiler.frontend.fdl.parser import Parser +from fory_compiler.frontend.proto.lexer import Lexer as ProtoLexer +from fory_compiler.frontend.proto.parser import Parser as ProtoParser +from fory_compiler.frontend.proto.translator import ProtoTranslator +from fory_compiler.frontend.fbs.lexer import Lexer as FbsLexer +from fory_compiler.frontend.fbs.parser import Parser as FbsParser +from fory_compiler.frontend.fbs.translator import FbsTranslator +from fory_compiler.ir.validator import SchemaValidator + + +def parse_fdl(source: str): + return Parser.from_source(source).parse() + + +def parse_proto(source: str): + lexer = ProtoLexer(source) + parser = ProtoParser(lexer.tokenize()) + return ProtoTranslator(parser.parse()).translate() + + +def parse_fbs(source: str): + lexer = FbsLexer(source) + parser = FbsParser(lexer.tokenize()) + return FbsTranslator(parser.parse()).translate() + + +def validate(schema): + validator = SchemaValidator(schema) + validator.validate() + return validator + + +def test_duplicate_service_names_fails_validation(): + source = """ + package test; + + service Alpha {} + service Alpha {} + """ + v = validate(parse_fdl(source)) + assert any("Duplicate service name: Alpha" in e.message for e in v.errors) + + +def test_unique_service_names_passes_validation(): + source = """ + package test; + + service Alpha {} + service Beta {} + """ + v = validate(parse_fdl(source)) + assert v.errors == [] + + +def test_duplicate_method_names_fails_validation(): + source = """ + package test; + + message Req {} + message Res {} + + service Greeter { + rpc SayHello (Req) returns (Res); + rpc SayHello (Req) returns (Res); + } + """ + v = validate(parse_fdl(source)) + assert any( + "Duplicate method name in service Greeter: SayHello" in e.message + for e in v.errors + ) + + +def test_same_method_name_in_different_services_passes_validation(): + source = """ + package test; + + message Req {} + message Res {} + + service Alpha { + rpc SayHello (Req) returns (Res); + } + + service Beta { + rpc SayHello (Req) returns (Res); + } + """ + v = validate(parse_fdl(source)) + assert v.errors == [] + + +def test_unique_method_names_passes_validation(): + source = """ + package test; + + message Req {} + message Res {} + + service Greeter { + rpc SayHello (Req) returns (Res); + rpc SayGoodbye (Req) returns (Res); + } + """ + v = validate(parse_fdl(source)) + assert v.errors == [] + + +def test_rpc_request_type_enum_fails_validation(): + source = """ + package test; + + enum Status { OK = 0; } + message Res {} + + service Svc { + rpc Call (Status) returns (Res); + } + """ + v = validate(parse_fdl(source)) + assert any( + "RPC type 'Status'" in e.message and "not a enum" in e.message + for e in v.errors + ) + + +def test_rpc_response_type_enum_fails_validation(): + source = """ + package test; + + message Req {} + enum Status { OK = 0; } + + service Svc { + rpc Call (Req) returns (Status); + } + """ + v = validate(parse_fdl(source)) + assert any( + "RPC type 'Status'" in e.message and "not a enum" in e.message + for e in v.errors + ) + + +def test_rpc_request_type_union_fails_validation(): + source = """ + package test; + + union Payload { string text = 1; } + message Res {} + + service Svc { + rpc Call (Payload) returns (Res); + } + """ + v = validate(parse_fdl(source)) + assert any( + "RPC type 'Payload'" in e.message and "not a union" in e.message + for e in v.errors + ) + + +def test_rpc_response_type_union_fails_validation(): + source = """ + package test; + + message Req {} + union Payload { string text = 1; } + + service Svc { + rpc Call (Req) returns (Payload); + } + """ + v = validate(parse_fdl(source)) + assert any( + "RPC type 'Payload'" in e.message and "not a union" in e.message + for e in v.errors + ) Review Comment: This assertion matches the exact wording "not a union". If error message wording is adjusted (e.g., for consistency with other validators), this will fail even though validation is correct. Prefer checking for stable substrings like "must be a message" and "union". ########## compiler/fory_compiler/ir/validator.py: ########## @@ -639,6 +640,37 @@ def check_message_fields( for f in union.fields: check_field(f, None) + def _check_services(self) -> None: + seen_service_names: dict = {} + for service in self.schema.services: + if service.name in seen_service_names: + self._error( + f"Duplicate service name: {service.name}", + service.location, + ) + seen_service_names.setdefault(service.name, service.location) + + seen_method_names: dict = {} + for method in service.methods: + if method.name in seen_method_names: + self._error( + f"Duplicate method name in service {service.name}: {method.name}", + method.location, + ) + seen_method_names.setdefault(method.name, method.location) Review Comment: _check_services() tracks prior locations in seen_service_names/seen_method_names but doesn’t use them when reporting duplicates. If the duplicate node has no location (or to match the existing duplicate-type checks), consider passing `service.location or seen_service_names[service.name]` (and similarly for methods) so the error is reliably anchored to a source location. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
