fix: prevent infinite recursion when extracting non-regular base class

[mizvekov]

A non-regular base class can have a member class which is a base class of the former.

This fix prevents infinite recursion in this case.

Fixes #1131

[github-actions]

🚧 Danger.js checks for MrDocs are experimental; expect some rough edges while we tune the rules.

✨ Highlights

• 🧪 New golden tests added

🧾 Changes by Scope

Scope	Lines Δ	Lines +	Lines -	Files Δ	Files +	Files ~	Files ↔	Files -
Golden Tests	164	164	-	5	5	-	-	-
Source	32	15	17	2	-	2	-	-
Total	196	179	17	7	5	2	-	-

Legend: Files + (added), Files ~ (modified), Files ↔ (renamed), Files - (removed)

🔝 Top Files

• test-files/golden-tests/symbols/record/forward-declared-member.html (Golden Tests): 89 lines Δ (+89 / -0)
• test-files/golden-tests/symbols/record/forward-declared-member.adoc (Golden Tests): 50 lines Δ (+50 / -0)
• src/lib/AST/ASTVisitor.cpp (Source): 27 lines Δ (+10 / -17)

Generated by 🚫 dangerJS against 0e1e5f9

[cppalliance-bot]

An automated preview of the documentation is available at https://1132.mrdocs.prtest2.cppalliance.org/index.html

If more commits are pushed to the pull request, the docs will rebuild at the same URL.

2025-12-12 17:07:11 UTC

[alandefreitas]

What seems good to me about this change is it seems correct that we should traverse rather than find because the symbol might not have been traversed yet. 😁 Maybe we should have been using findOrTraverse or one of these functions. Don't we have a findOrTraverse function or something like that that abstracts these two branches?

One minor point is the comment above

if (config_->inheritBaseMembers != PublicSettings::BaseMemberInheritance::Never)

probably needs to be rewritten then.

Then there's this part that is a bit confusing. 😅 I can't evaluate all the implications of this from the top of my mind, but it's a big smell because the whole point of the TraversalMode is to do the appropriate thing for each symbol type depending on what we're trying to achieve. Then we have the implications of a symbol pretending to be regular, which I can't easily evaluate.

My understanding is TraversalMode::BaseClass means "please also extract this even if it's not regular and without its members because we need it to represent a base class somewhere else" and the new code changes the semantics to "please also extract this even if it's not regular and without its members because we need it to represent a base class somewhere else but I'll temporarily pretend it's regular for you just in case there's a bug in your logic".

I can't comment on all the implications of pretending it's regular because there are so many implications I can think of (for instance, one implication is that regular symbols should have their members extracted. So, if anything, the fact that we extract more information from regular symbols should only increase the probability of recursion somewhere else?). From the code, I don't even know which aspect of regular symbols has the power to or is expected to prevent infinite recursion. Also, why doesn't the other case (when the symbol was is not there yet) doesn't have to pretend to be regular? Isn't this cirsumstancial to the test cases?

All I know is this probably isn't the best scope for the fix, since (i) it depends on some bug that seems circumstantial about the test cases and some bug in the regular symbol or traversal mode, (ii) it leaves the other traversal mode path broken, and (iii) it makes the happy path (almost?) always unused (almost always or always depending on a previous question).

In general, if this TraversalMode is not doing what we want to achieve for this symbol type, it is because the logic for this traversal mode is broken, and that's what has to be fixed. And this case seems even more special because TraversalMode::BaseClass only exists to be used here, and if we're making it void here, maybe there should be no reason for it to exist (which I also find very counter intuitive).

[mizvekov]

My understanding is TraversalMode::BaseClass means "please also extract this even if it's not regular and without its members because we need it to represent a base class somewhere else" and the new code changes the semantics to "please also extract this even if it's not regular and without its members because we need it to represent a base class somewhere else but I'll temporarily pretend it's regular for you just in case there's a bug in your logic".

The symbol is extracted with it's members, either with or without this patch, because we inject the members of the base class into the derived class.

I can't comment on all the implications of pretending it's regular because there are so many implications I can think of (for instance, one implication is that regular symbols should have their members extracted. So, if anything, the fact that we extract more information from regular symbols should only increase the probability of recursion somewhere else?). From the code, I don't even know which aspect of regular symbols has the power to or is expected to prevent infinite recursion. Also, why doesn't the other case (when the symbol was is not there yet) doesn't have to pretend to be regular? Isn't this cirsumstancial to the test cases?

Well the point of recursion is exactly here. For the test case included, if we take the non-regular branch, then that will leads us back here in the same situation and we will take the same branch again for the same symbol.

Changing the symbol to regular in this scope only has the implication that we will not take this branch the second time around, but it's otherwise the same amount of information extracted as far as I could see.

Another alternative is creating a new extraction mode, something like "ExtractedNonRegular", and just setting the symbol to that instead.

[alandefreitas]

The symbol is extracted with its members, either with or without this patch, because we inject the members of the base class into the derived class.

Yes. That makes sense. 🙈 I thought that would depend on what option we chose for inherit-base-members.

I don't know if the code changed since the last review. But the conditionals were aggregated, and because of the changes, the functional difference was tough to read. Split mode helped a little. 😁

So the changes are only isCompleteDefinition() and the IsBaseClass thing now, and my worries that the semantics of the traversal mode were wrong don't make sense anymore. 🙂

So my review can get much simpler: I like isCompleteDefinition() because there's no point when it fails 👍, and I don't like IsBaseClass because it can be replaced with much simpler logic that says "traverse this only if it hasn't been traversed yet". In fact, this logic is so common that we already have helper functions for that in the ASTVisitor. 😀

📝 Also, IsBaseClass is not a property of classes (unless the class is final, in which case it can't be a base class, but that's another discussion), the derived type would be RecordSymbol rather than a ClassSymbol, this would definitely not be a property of all symbols possible (it wouldn't go in SymbolBase), and it's using the DOM to store a local reminder related to ASTVisitor recursion only as a workaround for something where we couldn't find a better solution. Even without a better solution, we could store the set of base classes we have already visited in the ASTVisitor. Users don't want that information propagated to the generators because it's quasi-random, circumstantial information about which function demanded the symbol to be extracted first (if execution is multi-threaded, this could even be a different result each time). But all of that would be at another dialectical level, because there's more straightforward logic without storing any property in the classes for symbols.