[stable] Fix #21359 - Merge more types in `substWildTo()` by kinke · Pull Request #21361 · dlang/dmd

kinke · 2025-05-09T11:12:31Z

No description provided.

dlang-bot · 2025-05-09T11:12:35Z

Thanks for your pull request and interest in making D better, @kinke! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

In preparation for migrating from Bugzilla to GitHub Issues, the issue reference syntax has changed. Please add the word "Bugzilla" to issue references. For example, Fix Bugzilla Issue 12345 or Fix Bugzilla 12345.(Reminder: the edit needs to be done in the Git commit message, not the GitHub pull request.)

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "stable + dmd#21361"

…isn't merged yet

rainers · 2025-05-09T11:31:25Z

compiler/test/compilable/test21359.sh

+objfile="${OUTPUT_BASE}${OBJ}"
+$DMD -c -m${MODEL} -allinst -of${objfile} ${EXTRA_FILES}/${TEST_NAME}a.d -I${EXTRA_FILES}
+
+# This test is brittle and might break in the future.


How about checking for duplicate symbols in general:

! nm -j bla.o | uniq -d | read

worked for the test case.

I wanted to check that the symbol we are looking for is emitted once at least. E.g., in case -allinst in the future doesn't emit any TypeInfos for the test case, as none are actually required - no template is really instantiated in the root module.

Of course other fail_compilation tests now fail, with uglier 'forward reference' errors coming from the glue layer now, without a Loc. :(

I wanted to check that the symbol we are looking for is emitted once at least.

Ok.

Of course other fail_compilation tests now fail, with uglier 'forward reference' errors coming from the glue layer now, without a Loc. :(

~~These are messages from the glue layer.~~ I guess you can avoid them if the nextOf().merge().deco check is only added for case Taarray.

Oh, read your suggestion just now after pushing a version which doesn't do the nested merge() if .next is a TypeFunction. That seems to be enough to not worsen the existing forward-ref errors, but that special case is obviously ugly...

Edit: I did check whether #21350 might be enough too, but no, it doesn't seem to affect these forward-ref errors.

but that special case is obviously ugly...

I think it would be ok if implemented as a case Tfunction instead of extra checks in the generic case.

I'm checking type.next, not the type itself.

I guess it could be beneficial to move the if (type.deco) return type; before the switch though - the switch seems to be exclusively reserved for skipping the merge and returning type as-is (edit: modulo potential merge() calls for nested types, which can hopefully never affect type.deco).

Ah, sorry, missed that.

Then my suggestion to only change the AA case could be an option:

case Taarray: if (!type.isTypeAArray().index.merge().deco) return type; if (!type.nextOf().merge().deco) return type; break;

as I see deco being used in a lot of places to check for resolved semantic. More regressions looming...

The bad types come from makeNakedAssociativeArray. substWildTo doesn't return a merged type in some cases. Adding .merge() in makeNakedAssociativeArray helps bla.d, but probably fixing it in substWildTo is better:

dmd/compiler/src/dmd/typesem.d

Line 7349 in 8d90d93

return t;

shoud return t.merge().

Thx for digging further!

but probably fixing it in substWildTo is better

Yeah I guess there we also don't need a special case for TypeFunctions (no qualified TypeFunctions I hope, only qualified function pointers/delegates). After a superficial glance, those seem to be an ugly special case, e.g., there are 3 occurrences of this comment in that file:

dmd/compiler/src/dmd/typesem.d

Lines 2748 to 2753 in 8d90d93

/* Don't return merge(), because arg identifiers and default args

* can be different

* even though the types match

*/

mtype.deco = mtype.merge().deco;

return mtype;

Edit: Oh well, TypeFunction is hugely special-cased in that function already, should have taken a look at the linked context first. ;)

those seem to be an ugly special case, e.g., there are 3 occurrences of this comment in that file

Incidentally, substWildTo() does merge the TypeFunction it returns, not only copying the deco:

dmd/compiler/src/dmd/typesem.d

Lines 7378 to 7396 in 8d90d93

// Similar to TypeFunction.syntaxCopy;

auto t = new TypeFunction(ParameterList(params, tf.parameterList.varargs), tret, tf.linkage);

t.mod = ((tf.mod & MODFlags.wild) ? (tf.mod & ~MODFlags.wild) | MODFlags.const_ : tf.mod);

t.isNothrow = tf.isNothrow;

t.isNogc = tf.isNogc;

t.purity = tf.purity;

t.isProperty = tf.isProperty;

t.isRef = tf.isRef;

t.isReturn = tf.isReturn;

t.isReturnScope = tf.isReturnScope;

t.isScopeQual = tf.isScopeQual;

t.isReturnInferred = tf.isReturnInferred;

t.isScopeInferred = tf.isScopeInferred;

t.isInOutParam = false;

t.isInOutQual = false;

t.trust = tf.trust;

t.inferenceArguments = tf.inferenceArguments;

t.isCtor = tf.isCtor;

return t.merge();

FWIW, I've tried a t.deco = merge(t).deco; return t; instead, but that alone didn't fix the testcase.

rainers · 2025-05-09T14:17:27Z

compiler/src/dmd/typesem.d

-                return type;
+            if (auto next = type.nextOf())
+            {
+                // don't try to merge TypeFunctions (can shift forward-reference errors from the frontend to the glue layer, without Loc)


don't try to merge TypeFunctions

seems a bit misleading, maybe add "with incomplete return type"

According to my results (#21350 didn't fix it), I don't think it's restricted to those without a TypeFunction.next themselves.

rainers

Indeed, the special cases around deco make changes rather brittle. I guess this change is the one with the least unexpected side effects while making substWildTo more consistent.

Cherry-picked from #21361.

Fix dlang#21359 - Don't give up merging TypeNext too easily if .next …

d3c6a4d

…isn't merged yet

kinke force-pushed the fix21359 branch from 0c1a603 to d3c6a4d Compare May 9, 2025 11:22

rainers reviewed May 9, 2025

View reviewed changes

[make ugly exception for nested TypeFunction]

8d8a0d3

kinke marked this pull request as ready for review May 9, 2025 13:57

rainers reviewed May 9, 2025

View reviewed changes

[merge in substWildTo() instead]

2809a0f

rainers approved these changes May 9, 2025

View reviewed changes

kinke changed the title ~~[stable] Fix #21359 - Don't give up merging TypeNext too easily if .next isn't merged yet~~ [stable] Fix #21359 - Merge more types in substWildTo() May 10, 2025

kinke added a commit that referenced this pull request May 12, 2025

Fix #21359 - Merge more types in substWildTo()

7e1f8c1

Cherry-picked from #21361.

kinke added Merge:72h no objection -> merge The PR will be merged if there are no objections raised. Merge:auto-merge-squash labels May 15, 2025

dlang-bot merged commit 3bd2186 into dlang:stable May 19, 2025
86 of 87 checks passed

kinke deleted the fix21359 branch May 19, 2025 19:01

kinke mentioned this pull request May 19, 2025

[REG2.111] -allinst can cause corrupt TypeInfo_AssociativeArray symbols #21359

Closed

kinke added a commit to kinke/ldc that referenced this pull request Jun 4, 2025

Cherry-pick dlang/dmd#21361

289d365

	/* Don't return merge(), because arg identifiers and default args
	* can be different
	* even though the types match
	*/
	mtype.deco = mtype.merge().deco;
	return mtype;

	// Similar to TypeFunction.syntaxCopy;
	auto t = new TypeFunction(ParameterList(params, tf.parameterList.varargs), tret, tf.linkage);
	t.mod = ((tf.mod & MODFlags.wild) ? (tf.mod & ~MODFlags.wild) \| MODFlags.const_ : tf.mod);
	t.isNothrow = tf.isNothrow;
	t.isNogc = tf.isNogc;
	t.purity = tf.purity;
	t.isProperty = tf.isProperty;
	t.isRef = tf.isRef;
	t.isReturn = tf.isReturn;
	t.isReturnScope = tf.isReturnScope;
	t.isScopeQual = tf.isScopeQual;
	t.isReturnInferred = tf.isReturnInferred;
	t.isScopeInferred = tf.isScopeInferred;
	t.isInOutParam = false;
	t.isInOutQual = false;
	t.trust = tf.trust;
	t.inferenceArguments = tf.inferenceArguments;
	t.isCtor = tf.isCtor;
	return t.merge();

Uh oh!

Conversation

kinke commented May 9, 2025

Uh oh!

dlang-bot commented May 9, 2025

Bugzilla references

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

Testing this PR locally

Uh oh!

rainers May 9, 2025

Choose a reason for hiding this comment

Uh oh!

kinke May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rainers May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kinke May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rainers May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kinke May 9, 2025

Choose a reason for hiding this comment

Uh oh!

kinke May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rainers May 9, 2025

Choose a reason for hiding this comment

Uh oh!

kinke May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kinke May 9, 2025

Choose a reason for hiding this comment

Uh oh!

rainers May 9, 2025

Choose a reason for hiding this comment

Uh oh!

kinke May 9, 2025

Choose a reason for hiding this comment

Uh oh!

rainers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kinke May 9, 2025 •

edited

Loading

rainers May 9, 2025 •

edited

Loading

kinke May 9, 2025 •

edited

Loading

rainers May 9, 2025 •

edited

Loading

kinke May 9, 2025 •

edited

Loading

kinke May 9, 2025 •

edited

Loading