Skip to content

Fix handling of message argument type #317

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

Merged
merged 2 commits into from
Jul 24, 2025
Merged

Fix handling of message argument type #317

merged 2 commits into from
Jul 24, 2025

Conversation

goccy
Copy link
Member

@goccy goccy commented Jul 24, 2025

Since message types with aliases are mutually convertible, type conversion is also performed for message arguments.

@Copilot Copilot AI review requested due to automatic review settings July 24, 2025 06:26
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR improves the handling of message argument types by adding proper type conversion for message types with aliases. The key enhancement is recognizing that message types with aliases are mutually convertible and performing automatic type conversion for message arguments.

  • Adds a new type checking function isDifferentArgumentType that considers message aliases when comparing types
  • Updates type conversion generation to handle message arguments with alias conversions
  • Improves error messaging to show full type names instead of just type kinds

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
resolver/resolver.go Adds isDifferentArgumentType function with alias-aware type checking and updates error messages
resolver/message.go Extends type conversion declaration generation to handle message arguments
generator/code_generator.go Updates argument generation to use message argument field types as destination types
validator/validator_test.go Updates test expectations for improved error messages and new validation behavior
validator/testdata/invalid_message_argument.proto Adds test case for message argument type conflicts
generator/testdata/expected_simple_aggregation.go Updates generated code to reflect improved type conversion handling

@@ -5527,6 +5527,35 @@ func isDifferentType(from, to *Type) bool {
return from.Kind != to.Kind
}

Copy link
Preview

Copilot AI Jul 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function lacks documentation explaining its purpose, parameters, and how it differs from the existing isDifferentType function. Consider adding a comment explaining that it handles type compatibility for message arguments, including alias resolution.

Suggested change
// isDifferentArgumentType determines whether two types are incompatible as message arguments.
// It extends the logic of isDifferentType by handling type compatibility for message arguments,
// including alias resolution. Specifically, it checks for fully qualified names (FQDN) and
// resolves aliases using findMessageAliasName. This function is used in scenarios where
// argument-level type compatibility is required, rather than general type compatibility.

Copilot uses AI. Check for mistakes.

@@ -512,6 +512,23 @@ func (m *Message) TypeConversionDecls() []*TypeConversionDecl {
case def.Expr.Map != nil:
mapExpr := def.Expr.Map.Expr
switch {
case mapExpr.Message != nil:
Copy link
Preview

Copilot AI Jul 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The nested conditionals and repeated pattern 'msgExpr.Message != nil && msgExpr.Message.Rule != nil' create deeply nested code that is hard to follow. Consider extracting this logic into a separate helper function.

Copilot uses AI. Check for mistakes.

This comment has been minimized.

Copy link

Code Metrics Report

main (710717c) #317 (fb39ba1) +/-
Coverage 64.9% 65.0% +0.0%
Code to Test Ratio 1:0.3 1:0.3 -0.1
Test Execution Time 6m14s 6m21s +7s
Details
  |                     | main (710717c) | #317 (fb39ba1) |  +/-  |
  |---------------------|----------------|----------------|-------|
+ | Coverage            |          64.9% |          65.0% | +0.0% |
  |   Files             |             81 |             81 |     0 |
  |   Lines             |          14362 |          14401 |   +39 |
+ |   Covered           |           9333 |           9361 |   +28 |
- | Code to Test Ratio  |          1:0.3 |          1:0.3 |  -0.1 |
  |   Code              |          44253 |          44310 |   +57 |
+ |   Test              |          16983 |          16986 |    +3 |
- | Test Execution Time |          6m14s |          6m21s |   +7s |

Code coverage of files in pull request scope (79.9% → 79.8%)

Files Coverage +/- Status
generator/code_generator.go 77.3% -0.1% modified
resolver/cel.go 78.2% +1.2% modified
resolver/message.go 71.0% -0.1% modified
resolver/resolver.go 82.6% -0.2% modified

Reported by octocov

@goccy goccy merged commit f4e0a0f into main Jul 24, 2025
4 checks passed
@goccy goccy deleted the fix-msg-arg branch July 24, 2025 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants