Michal/diameter/fix-inheriting-of-indirect-inherits/otp-19626

[Mikaka27]

Fixes #8235

[github-actions]

CT Test Results

  2 files   36 suites   12m 59s ⏱️
151 tests 146 ✅ 5 💤 0 ❌
179 runs  174 ✅ 5 💤 0 ❌

Results for commit 0054504.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

• Complete CT logs (Download Logs)
• HTML Documentation (Download HTML Docs)
• No Windows Installer found

// Erlang/OTP Github Action Bot

[Copilot]

Pull Request Overview

This PR implements an indirect_inherits feature for the Diameter dictionary compiler that enables automatic recursive inheritance resolution. The feature ensures that when a dictionary inherits from another dictionary, all ancestors in the inheritance chain are automatically considered during code generation, eliminating the need to explicitly list all parent dictionaries.

• Adds new indirect_inherits option to the diameter compiler
• Implements automatic resolution of inheritance chains with proper handling of AVPs, enums, and vendor IDs
• Adds comprehensive test suites to verify inheritance behavior

Reviewed Changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
lib/diameter/test/modules.mk	Adds new test suites to the build configuration
lib/diameter/test/diameter_indirect_inherits_SUITE.erl	Comprehensive test suite for indirect inheritance functionality
lib/diameter/test/diameter_codegen_SUITE.erl	Test suite for code generation aspects of indirect inheritance
lib/diameter/test/diameter_compiler_SUITE.erl	Updates existing compiler tests to support indirect inherits
lib/diameter/src/compiler/diameter_make.erl	Adds documentation for the new indirect_inherits option
lib/diameter/src/compiler/diameter_dict_util.erl	Core implementation of inheritance chain resolution logic
lib/diameter/src/compiler/diameter_codegen.erl	Updates code generation to handle inherited enums properly
lib/diameter/doc/references/diameterc_cmd.md	Documents the new --indirect-inherits command line option
lib/diameter/doc/references/diameter_dict.md	Comprehensive documentation of inheritance behavior

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[lynxis]

So far I understand, you're fixing here a different thing, which could also fix this issue.
Let have the following example:

C.dia inherits B, defined Grouped Cesa which uses the Grouped Beta from B.dia in an own group.
But the Grouped Beta uses the AVP alpha from A.dia. B.dia inherits A.

c.dia

@inherits b
@vendor 10415 3GPP

@avp_types
   Cesa         1451    Grouped     MV

@grouped

Cesa ::= <AVP Header: 1451 10415>
   [ Beta ]

b.dia

@inherits a
@vendor 10415 3GPP
@avp_types
    Beta         1551    Grouped     MV

@grouped 
Beta ::= <AVP Header: 1551 10415>
   [ Alpha ]

a.dia

@vendor 10415 3GPP
@avp_types
    Alpha         1651    Unsigned32     MV

a.dia.txt
b.dia.txt
c.dia.txt

IMHO: the inherits are correctly assigned. C only reference to types of B and isn't using any A type. But B is using it.
EDIT: What I mean is, that this example of a.dia/... shouldn't need the new flag indirect-inherits, even for other cases it might be a good flag to use.

[pespin]

@Mikaka27 @lynxis with the reported testcase above, I can show how indeed it showcases a bug in my current Archlinux OTP (28.0.2-1). See how extra AVPs are added to the generated code when "@inherits a" is added explicitly to c.dia:

$ git diff --cached
diff --git a/dia/c.dia b/dia/c.dia
index 4dde94e..0bed9d3 100644
--- a/dia/c.dia
+++ b/dia/c.dia
@@ -1,3 +1,4 @@
+@inherits a
 @inherits b
 @vendor 10415 3GPP

diff --git a/src/c.erl b/src/c.erl
index 2f63113..f08c645 100644
--- a/src/c.erl
+++ b/src/c.erl
@@ -64,6 +64,7 @@ name2rec('Beta') -> 'Beta';
 name2rec(T) -> msg2rec(T).

 avp_name(1451, 10415) -> {'Cesa', 'Grouped'};
+avp_name(1651, 10415) -> {'Alpha', 'Unsigned32'};
 avp_name(1551, 10415) -> {'Beta', 'Grouped'};
 avp_name(_, _) -> 'AVP'.

@@ -76,11 +77,14 @@ avp_arity('Beta', 'Alpha') -> {0, 1};
 avp_arity(_, _) -> 0.

 avp_header('Cesa') -> {1451, 192, 10415};
+avp_header('Alpha') -> a:avp_header('Alpha');
 avp_header('Beta') -> b:avp_header('Beta');
 avp_header(_) -> erlang:error(badarg).

 avp(T, Data, 'Cesa', Opts) ->
     grouped_avp(T, 'Cesa', Data, Opts);
+avp(T, Data, 'Alpha', Opts) ->
+    avp(T, Data, 'Alpha', Opts, a);
 avp(T, Data, 'Beta', Opts) ->
     grouped_avp(T, 'Beta', Data, Opts);
 avp(_, _, _, _) -> erlang:error(badarg).
@@ -101,11 +105,13 @@ dict() ->
      {define, []},
      {enum, []},
      {grouped, [{"Cesa", 1451, [10415], [["Beta"]]}]},
-     {import_avps, [{b, [{"Beta", 1551, "Grouped", "MV"}]}]},
+     {import_avps,
+      [{a, [{"Alpha", 1651, "Unsigned32", "MV"}]},
+       {b, [{"Beta", 1551, "Grouped", "MV"}]}]},
      {import_enums, []},
      {import_groups,
       [{b, [{"Beta", 1551, [10415], [["Alpha"]]}]}]},
-     {inherits, [{"b", []}]},
+     {inherits, [{"b", []}, {"a", []}]},
      {messages, []},
      {vendor, {10415, "3GPP"}}].

[pespin]

@Mikaka27 I tried the same a,b,c testcase under rebar3 using OTP built out of this PR and I get the exact same result, so this PR is imho not fixing the problem.

[Mikaka27]

@Mikaka27 I tried the same a,b,c testcase under rebar3 using OTP built out of this PR and I get the exact same result, so this PR is imho not fixing the problem.

Hi, could you share the test case along with commands you used where it doesn't work as you expect? Thanks :)

[pespin]

@Mikaka27 see #8235 (comment) , my fault due to not passing indirect_inherits, the PR is fine!

[Mikaka27]

So far I understand, you're fixing here a different thing, which could also fix this issue. Let have the following example:

C.dia inherits B, defined Grouped Cesa which uses the Grouped Beta from B.dia in an own group. But the Grouped Beta uses the AVP alpha from A.dia. B.dia inherits A.

c.dia

@inherits b
@vendor 10415 3GPP

@avp_types
   Cesa         1451    Grouped     MV

@grouped

Cesa ::= <AVP Header: 1451 10415>
   [ Beta ]

b.dia

@inherits a
@vendor 10415 3GPP
@avp_types
    Beta         1551    Grouped     MV

@grouped 
Beta ::= <AVP Header: 1551 10415>
   [ Alpha ]

a.dia

@vendor 10415 3GPP
@avp_types
    Alpha         1651    Unsigned32     MV

a.dia.txt b.dia.txt c.dia.txt

IMHO: the inherits are correctly assigned. C only reference to types of B and isn't using any A type. But B is using it. EDIT: What I mean is, that this example of a.dia/... shouldn't need the new flag indirect-inherits, even for other cases it might be a good flag to use.

Hi, is that a different problem you're having, other then @pespin or is that also resolved when using the indirect-inherits option?

[bmk]

"DCOMP" should be "DII" I think.

[Mikaka27]

Fixed

[bmk]

Only cosmetic comments.