Fix: core.stdcpp.vector cannot have core.stdcpp.vector as element #3238

n8sh · 2020-10-17T19:09:51Z

Only pertains to 64-bit Windows since that's the only platform currently supported by core.stdcpp.vector.

dlang-bot · 2020-10-17T19:09:53Z

Thanks for your pull request, @n8sh!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	21323	normal	(64-bit Windows only) core.stdcpp.vector could not have core.stdcpp.vector as element

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 + druntime#3238"

TurkeyMan · 2020-10-17T21:42:38Z

src/core/stdcpp/vector.d

@@ -587,7 +587,7 @@ extern(D):
                for (size_t i = _Size; i > 0; )
                {
                    --i;
-                    _Get_data()._Myfirst[i].moveEmplace(_Newvec[i]);
+                    moveEmplace(_Get_data()._Myfirst[i], _Newvec[i]);


O_o ... what's going on here? If that fixes a bug, then I'm worried there's an issue with the language...

Based on your recent comment, I think this fix is hiding the issue, and you should revert this change and fix the issue instead.

That would fix this specific case but a related problem could still arise for any struct with a public member named moveEmplace.

That might not be a problem. It could be a member function with a contract compatible with core.lifetime.moveEmplace which may have legitimate reason to exist and using it instead of the expected function could be beneficial. Against this I'll note that move and core_emplace aren't used inside vector with UFCS. That style was only used with that single call to moveEmplace.

Please confirm you have read this and still want to retain UFCS here. If so I will change the PR as requested.

could still arise for any struct with a public member named moveEmplace

Oh yeah, of course >_<

Perhaps both fixes are necessary... although it depends on if it's decided that creating a moveEmplace local to the object as a global override is a desired feature or not? Should it be considered that an object's local version is an intent to override the global? I think that needs to be resolved broadly.

Sadly, moveEmplace is a gross hack in D, and I'm not sure I would trust anyone to write it, but this pattern should should be decided and applied broadly.

I'm quite certain that it's improper for vector to have those local imports at member-scope, but the fix to this line can be seen from either perspective as you've shown :/

Updated the PR to apply both fixes.

TurkeyMan

LGTM... but why does this work? Is there a larger issue?

n8sh · 2020-10-17T22:31:49Z

A similar problem would arise with UFCS for any type with a visible member with the same name as the function. The specific issue here is that vector privately imports moveEmplace andvector.moveEmplace is visible to the other vector.

thewilsonator · 2020-10-19T01:56:49Z

git clone -b v2.094.1 --depth=1 https://github.com/dlang/druntime .generated/druntime-2.094.1
Cloning into '.generated/druntime-2.094.1'...
warning: Could not find remote branch v2.094.1 to clone.
fatal: Remote branch v2.094.1 not found in upstream origin

This is probably due to the issues merging stable.

TurkeyMan · 2020-10-19T03:24:22Z

The specific issue here is that vector privately imports moveEmplace andvector.moveEmplace is visible to the other vector.

That is clearly the issue here... an API should not be designed to not work with UFCS.
This fix should instead use a renaming import when importing moveEmplace, or change the location of the import so this can't occur.
This issue you demonstrate here actually suggests to me that it might be general bad practise to use local imports in struct/class scopes, because it may interfere with UFCS in a surprising and undesirable way. I hadn't encountered this issue before, but I think it will necessitate my choosing to not ever import into public struct scope in the future.

TurkeyMan

I think the imports should be moved to be function-local, or module scoped, or they could be renamed, or maybe they could be private? (not sure if making them private would actually solve the issue?)

TurkeyMan · 2020-10-19T03:25:50Z

src/core/stdcpp/vector.d

@@ -587,7 +587,7 @@ extern(D):
                for (size_t i = _Size; i > 0; )
                {
                    --i;
-                    _Get_data()._Myfirst[i].moveEmplace(_Newvec[i]);
+                    moveEmplace(_Get_data()._Myfirst[i], _Newvec[i]);


Based on your recent comment, I think this fix is hiding the issue, and you should revert this change and fix the issue instead.

n8sh · 2020-10-19T17:36:01Z

maybe they could be private? (not sure if making them private would actually solve the issue?)

They are already private (that is now the default), the issue here being that vector!A can see the private imports of vector!B. In any file except core.stdcpp.vector.d there would be no problem using UFCS to call moveEmplace on a vector.

…have core.stdcpp.vector as element

TurkeyMan · 2020-10-20T09:07:37Z

src/core/stdcpp/vector.d

@@ -32,7 +32,7 @@ extern(C++, "std"):

 extern(C++, class) struct vector(T, Alloc = allocator!T)
 {
-    import core.lifetime : forward, move, moveEmplace, core_emplace = emplace;
+    import core.lifetime : forward, move, core_emplace = emplace;


I think it's likely that all of these will trigger the same problem... no?
auto newVector = myVector.move(); <- same issue?

It works fine because UFCS isn't used for those inside vector and outside vector the imports aren't visible.

How is it not visible? Those imports are public, and definitely visible.

import core.lifetime : move; struct Test { import core.lifetime : move; } int main() { Test t; auto u = move(t); <- works auto v = t.move(); <- fails, because sees the local one, and thinks it has no arguments return 0; }

Basically, when you import anything into struct/class scope, it seems to be preferred over UFCS.

Oh I see, you're saying the imports are private by default. My test is within a module.
Yeah, fair enough, but the whole thing kinda smells to me :/
The same problems may occur within a module, but less likely, and at least you can control the situation in that case.

12345swordy · 2020-10-25T00:46:32Z

All good to go?

TurkeyMan · 2020-10-25T03:52:55Z

Do it

n8sh requested review from andralex, TurkeyMan, WalterBright and wilzbach as code owners October 17, 2020 19:09

dlang-bot added the Bug Fix Include reference to corresponding bugzilla issue label Oct 17, 2020

TurkeyMan reviewed Oct 17, 2020

View reviewed changes

TurkeyMan approved these changes Oct 17, 2020

View reviewed changes

TurkeyMan suggested changes Oct 19, 2020

View reviewed changes

Fix Issue 21323 - (64-bit Windows only) core.stdcpp.vector could not …

4365119

…have core.stdcpp.vector as element

n8sh force-pushed the issue-21323 branch from aeb263a to 4365119 Compare October 20, 2020 08:07

TurkeyMan reviewed Oct 20, 2020

View reviewed changes

TurkeyMan approved these changes Oct 25, 2020

View reviewed changes

thewilsonator added the auto-merge label Oct 25, 2020

dlang-bot merged commit 444fc36 into dlang:stable Oct 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: core.stdcpp.vector cannot have core.stdcpp.vector as element #3238

Fix: core.stdcpp.vector cannot have core.stdcpp.vector as element #3238

n8sh commented Oct 17, 2020

dlang-bot commented Oct 17, 2020

TurkeyMan Oct 17, 2020

TurkeyMan Oct 19, 2020

n8sh Oct 19, 2020

TurkeyMan Oct 19, 2020

n8sh Oct 20, 2020

TurkeyMan left a comment

n8sh commented Oct 17, 2020

thewilsonator commented Oct 19, 2020

TurkeyMan commented Oct 19, 2020

TurkeyMan left a comment

TurkeyMan Oct 19, 2020

n8sh commented Oct 19, 2020

TurkeyMan Oct 20, 2020 •

edited

Loading

n8sh Oct 20, 2020

TurkeyMan Oct 20, 2020

TurkeyMan Oct 20, 2020

12345swordy commented Oct 25, 2020

TurkeyMan commented Oct 25, 2020

Fix: core.stdcpp.vector cannot have core.stdcpp.vector as element #3238

Fix: core.stdcpp.vector cannot have core.stdcpp.vector as element #3238

Conversation

n8sh commented Oct 17, 2020

dlang-bot commented Oct 17, 2020

Bugzilla references

Testing this PR locally

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TurkeyMan left a comment

Choose a reason for hiding this comment

n8sh commented Oct 17, 2020

thewilsonator commented Oct 19, 2020

TurkeyMan commented Oct 19, 2020

TurkeyMan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n8sh commented Oct 19, 2020

TurkeyMan Oct 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

12345swordy commented Oct 25, 2020

TurkeyMan commented Oct 25, 2020

TurkeyMan Oct 20, 2020 •

edited

Loading