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

Mikaka27 · 2025-08-29T15:54:21Z

Fixes #8235

github-actions · 2025-08-29T15:55:05Z

CT Test Results

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

Results for commit 08fb796.

♻️ 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

// 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.}

Copilot · 2025-08-29T16:11:15Z

lib/diameter/test/diameter_indirect_inherits_SUITE.erl

+         end_per_suite/1,
+         init_per_testcase/2,
+         end_per_testcase/2,
+


Remove the trailing whitespace on this line.

Suggested change

Copilot · 2025-08-29T16:11:16Z

lib/diameter/test/diameter_codegen_SUITE.erl

+         end_per_suite/1,
+         init_per_testcase/2,
+         end_per_testcase/2,
+


Remove the trailing whitespace on this line.

Suggested change

lib/diameter/test/diameter_indirect_inherits_SUITE.erl

Copilot · 2025-08-29T16:11:17Z

lib/diameter/src/compiler/diameter_dict_util.erl

+% inherited_modules/2
+% Returns list of inherited modules, without returning module in which avp is defined


The comment style is inconsistent - use %% for function documentation comments to match the rest of the file.

Suggested change

% inherited_modules/2

% Returns list of inherited modules, without returning module in which avp is defined

%% inherited_modules/2

%% Returns list of inherited modules, without returning module in which avp is defined

Copilot · 2025-08-29T16:11:17Z

lib/diameter/src/compiler/diameter_dict_util.erl

+            NestedInherits = lists:flatmap(fun([]) ->
+                                                   [];
+                                              ({Mod, _}) ->
+                                                   get_all_inherits_from_module(dict(?A(Mod)))
+                                           end, Inherits),


[nitpick] The pattern matching for empty list ([]) -> [] could be simplified by filtering out empty lists before the flatmap operation for better readability.

Suggested change

NestedInherits = lists:flatmap(fun([]) ->

[];

({Mod, _}) ->

get_all_inherits_from_module(dict(?A(Mod)))

end, Inherits),

NestedInherits = lists:flatmap(

fun({Mod, _}) ->

get_all_inherits_from_module(dict(?A(Mod)))

end,

[I || I <- Inherits, I =/= []]

),

lynxis · 2025-09-03T15:17:13Z

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 · 2025-09-03T15:50:48Z

@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 · 2025-09-03T15:58:35Z

@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 · 2025-09-03T16:09:42Z

@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 · 2025-09-03T19:37:41Z

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

Mikaka27 · 2025-09-08T07:22:03Z

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?

Mikaka27 added 3 commits August 29, 2025 17:51

Add indirect-inherits option

a61fa3c

Add documentation about indirect-inherits option

7d373b8

Add tests for indirect-inherits option

08fb796

Mikaka27 requested a review from Copilot August 29, 2025 16:10

Copilot AI reviewed Aug 29, 2025

View reviewed changes

Mikaka27 self-assigned this Aug 29, 2025

Mikaka27 added team:PS Assigned to OTP team PS testing currently being tested, tag is used by OTP internal CI labels Aug 29, 2025

Mikaka27 mentioned this pull request Sep 1, 2025

Diameter .dia not automatically inheriting indirect inherits #8235

Open

Mikaka27 removed the testing currently being tested, tag is used by OTP internal CI label Sep 2, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Mikaka27 commented Aug 29, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Aug 29, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Aug 29, 2025

Uh oh!

Copilot AI Aug 29, 2025

Uh oh!

Uh oh!

Copilot AI Aug 29, 2025

Uh oh!

Copilot AI Aug 29, 2025

Uh oh!

lynxis commented Sep 3, 2025 •

edited

Loading

Uh oh!

pespin commented Sep 3, 2025

Uh oh!

pespin commented Sep 3, 2025

Uh oh!

Mikaka27 commented Sep 3, 2025

Uh oh!

pespin commented Sep 3, 2025

Uh oh!

Mikaka27 commented Sep 8, 2025

c.dia

b.dia

a.dia

Uh oh!

Uh oh!

		% inherited_modules/2
		% Returns list of inherited modules, without returning module in which avp is defined

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

Are you sure you want to change the base?

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

Conversation

Mikaka27 commented Aug 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Aug 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CT Test Results

Artifacts

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Aug 29, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 29, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Aug 29, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 29, 2025

Choose a reason for hiding this comment

Uh oh!

lynxis commented Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

c.dia

b.dia

a.dia

Uh oh!

pespin commented Sep 3, 2025

Uh oh!

pespin commented Sep 3, 2025

Uh oh!

Mikaka27 commented Sep 3, 2025

Uh oh!

pespin commented Sep 3, 2025

Uh oh!

Mikaka27 commented Sep 8, 2025

c.dia

b.dia

a.dia

Uh oh!

Uh oh!

Mikaka27 commented Aug 29, 2025 •

edited

Loading

github-actions bot commented Aug 29, 2025 •

edited

Loading

lynxis commented Sep 3, 2025 •

edited

Loading