[REG2.068.0] Issue 15726 - forward reference error for circular classes, RefCounted #5500

9rnsr · 2016-03-04T13:33:52Z

Defer aggregate instance size finalization until semantic2 at maximum

From long ago, an instance size finalization had been done during semantic() stage, but it had had a known problem. When an aggregate 'A' tries to build its dtor or postblit, they should know each types of instance fields. But the field type analysis could recursively reach to A's sematnic() (especially with RefCounted, Variant, etc) and it had caused forward reference issues.

Against that, we've tried many ways to detect/break the problematic recursive analysis. So far it has been mostly finished to monitoring sizeok field, though it has been still fragile.

Finally I found an essential solution. During the dtor or postblit building, we don't have to finalize instance size. Instead we just need to know the complete list of instance fields at that time.

From the view, I divide instance size determination process into two stages.

The first is determineFields(), it collects all instance fields then pushes into AggregateDeclaration.fields. It's called in aggregate's semantic() and makes preparations for buildDtor() and buildPostBlit().

The second is determineSize(), it requests finalizing each type sizes of instance fields and determine the field offsets. If no declarations' semantic() depend on the instance size, determineSize() could be deferred until semantic2() stage at maximum.

Finally, a failure of determineSize() can mean an existence of unresolvable forward references (circular dependency, use of opaque struct, etc).

This PR is intentionally based on master branch, because this change is too big as a regression fix.

dlang-bot · 2016-03-04T13:33:54Z

Fix	Bugzilla	Description
✗	14666	[REG2.061] Bogus forward reference error
✓	15726	[REG2.068.0] forward reference error for circular classes, RefCounted

9rnsr · 2016-03-08T15:23:24Z

Ready to review.

WalterBright · 2016-03-15T07:19:03Z

Please do refactorings as separate PRs.

WalterBright · 2016-03-15T07:57:55Z

I've been repeating this point many times over the last few months, so I wrote this:
http://wiki.dlang.org/Starting_as_a_Contributor#Characteristics_of_a_Good_Pull_Request

9rnsr · 2016-03-16T01:43:35Z

Now this PR contains only core changes for issue 15726.

MartinNowak · 2016-03-20T09:36:10Z

src/clone.d

+                n *= (cast(TypeSArray)tv).dim.toUInteger();
+                tv = tv.nextOf().toBasetype();
+            }
+            if (n == 0)


We should have a small helper function for that in TypeSArray.

Not bad, but today it will be used only at two places.

WalterBright · 2016-03-31T03:35:03Z

src/aggregate.d

+            if (v.storage_class & (STCstatic | STCextern | STCtls | STCgshared | STCmanifest | STCctfe | STCtemplateparameter))
+                return 0;
+            if (!v.isField() || v.sem < SemanticDone)
+                return 1;   // unresolvable forward referernce


reference spelling

WalterBright · 2016-03-31T03:48:53Z

src/dstruct.d

-            structsize = 0;
-            alignsize = 0;
-
+            assert(type == Type.terror);


type.ty == Terror

CyberShadow · 2016-03-31T07:10:12Z

Please do refactorings as separate PRs.

BTW,

GitHub has now improved the commit viewer in pull requests. Now, there are prev/next buttons, so that pull requests can contain logically self-contained changes split up in commits, and reviewing can be done piecewise by looking at individual commits. This makes it easy to ignore refactoring / whitespace changes when reviewing PRs, so maybe that rule is not as necessary any more?

WalterBright · 2016-03-31T07:54:50Z

http://wiki.dlang.org/Starting_as_a_Contributor#Characteristics_of_a_Good_Pull_Request

I really don't understand the resistance to having small, one topic, focused PRs, especially when they are complex. Even this one was unnecessarily difficult because a lot of the diff lines were still refactorings unrelated to the purpose of the PR, and the github diff view still displayed them. I still wound up looking at 10 line blocks wondering "is this a refactoring, or is it substantive?"

A good PR should do one thing, for darn good reasons. It's analogous to not liking spaghetti code, and unencapsulated data structures. We certainly have plenty of regressions from PRs.

It's not like there aren't bugs in the refactorings. There is a history of them.

CyberShadow · 2016-03-31T07:59:37Z

Well, I'm asking because I don't understand the resistance against it. In particular...

I still wound up looking at 10 line blocks wondering "is this a refactoring, or is it substantive?"

Well, that's what looking at individual commits is supposed to solve, isn't it?

A good PR should do one thing, for darn good reasons.

Could you please explain how splitting changes into logically separate PRs is different from splitting changes into logically separate commits?

It's analogous to not liking spaghetti code, and unencapsulated data structures.

I don't see how that analogy holds, because the changes are encapsulated, but in commits, not PRs.

As for the arguments for this, well, if you need to refactor something to fix something else, then the refactoring is blocking your other work, which I can understand to be frustrating - by the time your refactoring PR has been reviewed and merged, you're elsewhere, and have to back up to submit the actual fix. That's my understanding of the situation, and I'm mainly asking out of curiosity anyway.

yebblies · 2016-03-31T08:57:29Z

I don't see how that analogy holds, because the changes are encapsulated, but in commits, not PRs.

Looking at the commits one-by-one is harder, because you still need to load the full thing into your head. Quite often it's not obvious which commits depend on which other commits, or at which points in the middle the code still works. Sometimes code is added in one commit and removed in another. With multiple PRs there is no question.

The PR author already knows all this information, and they can split into multiple PRs in much less time than it would take me to reconstruct this in my head. Especially if I'm doing it in 10 minute chunks here and there when I have time to read PRs. I assume others are in a similar situation.

As for the arguments for this, well, if you need to refactor something to fix something else, then the refactoring is blocking your other work, which I can understand to be frustrating - by the time your refactoring PR has been reviewed and merged, you're elsewhere, and have to back up to submit the actual fix. That's my understanding of the situation, and I'm mainly asking out of curiosity anyway.

I've never found that to be a significant problem in practice. Rebasing, pushing only some commits from a branch, etc are usually pretty easy.

In dlang#5500, issue 14666 has been properly fixed by adding mechanism of deferred semantic2 running for imported modules. Now, the workaround in `todt.c` is not necessary anymore.

In dlang#5500, issue 14666 has been properly fixed by adding mechanism of deferred semantic2 running for imported modules. Now, the workaround for issue 9057 in `todt.c` is unnecessary anymore.

CyberShadow · 2016-10-11T08:05:35Z

This pull request introduced a regression:
https://issues.dlang.org/show_bug.cgi?id=16607

MartinNowak · 2016-10-19T23:12:16Z

And another one Issue 16627 – [Reg 2.072] struct with disabled postblit field is now nested.

MartinNowak · 2016-10-19T23:27:39Z

More specifically both regressions are caused by the 2nd commit of this PR 75b5b69.

…sted - Since dlang#5500 buildPostblit/Dtor/OpAssign are run before the struct size is finalized, thereby now making structs nested that previously were not. - Not sure if the old behavior was an intended feature. If not we might want to deprecate it.

…sted - Since dlang/dmd#5500 buildPostblit/Dtor/OpAssign are run before the struct size is finalized, thereby now making structs nested that previously were not. - Not sure if the old behavior was an intended feature. If not we might want to deprecate it.

ibuclaw · 2017-02-10T08:49:05Z

And another regression where test13613.S1 puts out 4 fields, instead of 3 because there's no protection against recursive calls to determineFields. Which looks like it is fixed by #5773.

ibuclaw · 2017-02-10T08:52:56Z

Specifically, this commit, which adds recursive protection. 80d1398

CyberShadow · 2017-07-03T00:13:14Z

This pull request introduced a regression:
https://issues.dlang.org/show_bug.cgi?id=17307

CyberShadow · 2018-03-13T15:58:39Z

This pull request introduced a regression:
https://issues.dlang.org/show_bug.cgi?id=18606

CyberShadow · 2020-04-20T04:19:20Z

This pull request may have introduced a regression:
https://issues.dlang.org/show_bug.cgi?id=20753

9rnsr force-pushed the fix15726 branch 6 times, most recently from 5034590 to 89213a9 Compare March 8, 2016 14:41

9rnsr mentioned this pull request Mar 9, 2016

speed up slow quadratic search in addDeferred #5506

Merged

9rnsr force-pushed the fix15726 branch from 89213a9 to 69c392a Compare March 15, 2016 11:04

This was referenced Mar 15, 2016

Fix debug printf statements for D #5529

Merged

Remove Scope parameter from finalizeSize() #5532

Merged

9rnsr force-pushed the fix15726 branch 2 times, most recently from b51aeaf to 44e7f88 Compare March 16, 2016 01:39

9rnsr force-pushed the fix15726 branch 2 times, most recently from f192bd9 to bf191c7 Compare March 20, 2016 03:03

MartinNowak reviewed Mar 20, 2016
View reviewed changes

9rnsr force-pushed the fix15726 branch 2 times, most recently from 2c417dd to a48df86 Compare March 29, 2016 08:34

WalterBright mentioned this pull request Mar 29, 2016

refactor: replace dt_t* with DtBuilder (part 2) #5600

Merged

WalterBright reviewed Mar 31, 2016
View reviewed changes

Do not request field type.size() in buildDtor and buildPostBlit

d87f8f9

WalterBright reviewed Mar 31, 2016
View reviewed changes

9rnsr deleted the fix15726 branch March 31, 2016 07:50

9rnsr mentioned this pull request Apr 24, 2016

Remove workaround for issue 9057 #5700

Merged

This was referenced May 12, 2016

Refactor: add AggregateDeclaration.newScope to factor out common code #5768

Merged

Issue 16011, 16012, 16013 - Improve forward reference resolution during field type analysis #5773

Open

9rnsr mentioned this pull request May 29, 2016

[REG2.072a] Issue 15992 - ICE with field variable of instantiated struct #5780

Merged

MartinNowak mentioned this pull request Oct 24, 2016

fix Issue 16627 - non-static structs with postblit/dtor fields now nested #6212

Merged

ibuclaw self-assigned this Mar 13, 2018

WalterBright mentioned this pull request Dec 21, 2018

switch from Target. to instance target. #9108

Merged

ibuclaw mentioned this pull request Apr 20, 2020

partially address Issue 19722 - botched implementation of semantic3Er… #9425

Merged

ibuclaw mentioned this pull request May 5, 2020

[WIP] Attempt stabilizing order of semantic analysis in forward referenced structs #11099

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REG2.068.0] Issue 15726 - forward reference error for circular classes, RefCounted #5500

[REG2.068.0] Issue 15726 - forward reference error for circular classes, RefCounted #5500

9rnsr commented Mar 4, 2016

dlang-bot commented Mar 4, 2016

9rnsr commented Mar 8, 2016

WalterBright commented Mar 15, 2016

WalterBright commented Mar 15, 2016

9rnsr commented Mar 16, 2016

MartinNowak Mar 20, 2016

9rnsr Mar 20, 2016

WalterBright Mar 31, 2016

9rnsr Mar 31, 2016

WalterBright Mar 31, 2016

9rnsr Mar 31, 2016

CyberShadow commented Mar 31, 2016

WalterBright commented Mar 31, 2016

CyberShadow commented Mar 31, 2016

yebblies commented Mar 31, 2016

CyberShadow commented Oct 11, 2016

MartinNowak commented Oct 19, 2016 •

edited

Loading

MartinNowak commented Oct 19, 2016

ibuclaw commented Feb 10, 2017 •

edited

Loading

ibuclaw commented Feb 10, 2017

CyberShadow commented Jul 3, 2017

CyberShadow commented Mar 13, 2018

CyberShadow commented Apr 20, 2020

[REG2.068.0] Issue 15726 - forward reference error for circular classes, RefCounted #5500

[REG2.068.0] Issue 15726 - forward reference error for circular classes, RefCounted #5500

Conversation

9rnsr commented Mar 4, 2016

dlang-bot commented Mar 4, 2016

9rnsr commented Mar 8, 2016

WalterBright commented Mar 15, 2016

WalterBright commented Mar 15, 2016

9rnsr commented Mar 16, 2016

MartinNowak Mar 20, 2016

Choose a reason for hiding this comment

9rnsr Mar 20, 2016

Choose a reason for hiding this comment

WalterBright Mar 31, 2016

Choose a reason for hiding this comment

9rnsr Mar 31, 2016

Choose a reason for hiding this comment

WalterBright Mar 31, 2016

Choose a reason for hiding this comment

9rnsr Mar 31, 2016

Choose a reason for hiding this comment

CyberShadow commented Mar 31, 2016

WalterBright commented Mar 31, 2016

CyberShadow commented Mar 31, 2016

yebblies commented Mar 31, 2016

CyberShadow commented Oct 11, 2016

MartinNowak commented Oct 19, 2016 • edited Loading

MartinNowak commented Oct 19, 2016

ibuclaw commented Feb 10, 2017 • edited Loading

ibuclaw commented Feb 10, 2017

CyberShadow commented Jul 3, 2017

CyberShadow commented Mar 13, 2018

CyberShadow commented Apr 20, 2020

MartinNowak commented Oct 19, 2016 •

edited

Loading

ibuclaw commented Feb 10, 2017 •

edited

Loading