fix issues 22130, 17635 - pure factory functions #12890

aG0aep6G · 2021-07-19T10:46:54Z

Issue 22130 - [REG2.080.1][DIP1000] pure factory functions stopped working
Issue 17635 - [REG 2.066.0] cannot convert unique immutable(int)** to
immutable

Immutable parameters are not needed for strong purity. Const is enough.
Mutability in the return type is an additional condition on top of strong
purity when considering side effects.

The test case for issue 17635 was inverted without good reason in
#8048. Flipping again to revert that
mistake.

dlang-bot · 2021-07-19T10:46:59Z

Thanks for your pull request and interest in making D better, @aG0aep6G! 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

Auto-close	Bugzilla	Severity	Description
✓	17635	regression	[REG 2.066.0] cannot convert unique immutable(int)** to immutable
✓	22130	regression	[REG2.080.1][DIP1000] pure factory functions stopped working

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

Regression or critical bug fixes should always target the stable branch. Learn more about rebasing to stable or the D release process.

To target stable perform these two steps:

Rebase your branch to upstream/stable:

git rebase --onto upstream/stable upstream/master

Change the base branch of your PR to stable

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 "master + dmd#12890"

thewilsonator

This PR breaks a number of projects on buildkite (with warnings as errors), most of the error seem to be of the form

src\object.d(3824): Warning: calling `object.dup!(Sunsafe).dup` without side effects discards return value of type `Sunsafe[]`; prepend a `cast(void)` if intentional
src\object.d(3875): Warning: calling `object.dup!(Sunsafe).dup` without side effects discards return value of type `Sunsafe[]`; prepend a `cast(void)` if intentional
src/dmd/link.d(707): Warning: calling `dmd.root.rmem.Mem.error` without side effects discards return value of type `void*`; prepend a `cast(void)` if intentional
/src/ocean/core/SmartUnion.d(549): Warning: calling `ocean.core.SmartUnion.SmartUnion!(HasDuplicates).SmartUnion.opCall` without side effects discards return value of type `SmartUnion!(HasDuplicates)`; prepend a `cast(void)` if intentional

and a number of segfaults (which may just be OOMs)

src/dmd/frontend.h

src/dmd/mtype.d

thewilsonator · 2021-07-19T12:04:57Z

the src/dmd/link.d(707) warning seems to come from an unconditional assert(0)

    static void* error() pure nothrow @nogc @safe
    {
        onOutOfMemoryError();
        assert(0);
    }

used like

    static void* check(void* p) pure nothrow @nogc
    {
        return p ? p : error();
    }

Not sure about the other.

thewilsonator · 2021-07-19T12:13:30Z

src/dmd/link.d(707) seems to be the only caller of root.rmem.Mem.error() so if you want to change

dmd/src/dmd/link.d

Lines 705 to 707 in 1a1b81d

    
           auto buf = (cast(char*) malloc(bufsize))[0 .. bufsize]; 
        
           if (!buf) 
        
               Mem.error();

to a call to Mem.check() that should fix that problem

aG0aep6G · 2021-07-19T14:26:00Z

src/dmd/link.d(707) seems to be the only caller of root.rmem.Mem.error() so if you want to change

dmd/src/dmd/link.d

Lines 705 to 707 in 1a1b81d

auto buf = (cast(char*) malloc(bufsize))[0 .. bufsize];

if (!buf)

Mem.error();

to a call to Mem.check() that should fix that problem

Sounds good. Done.

thewilsonator · 2021-07-20T00:19:29Z

Dependant PRs merged

aG0aep6G · 2021-07-20T19:06:14Z

Some test results seem to be outdated. E.g., the Azure pipelines show warnings in druntime that I already addressed.

Others I can't reproduce locally. E.g., CyberShadow/DAutoTest says that dlang.org's test_dspec fails with "double free or corruption", but it works for me locally.

How do I push this forward?

thewilsonator · 2021-07-20T23:27:52Z

Im pretty sure you'll need to restart the pipelines by rebasing (or force pushing) as for the doublefree IDK

aG0aep6G · 2021-07-21T20:56:54Z

The outdated test results are gone. But I'm still stuck at the ones I can't reproduce locally.

Looking at buildkite, there seems to be memory corruption related to tempCString ("Failed to stat file [garbage]"). That could happen if a tempCString were wrongly reused, which is something that this PR might introduce. But I can't tell how it happens, and I can't reproduce the error.

dkorpel · 2021-11-13T16:29:52Z

@aG0aep6G Now that #13047 is merged, could you rebase this? I'm hoping the segfault goes away.

aG0aep6G · 2021-11-14T11:35:22Z

@aG0aep6G Now that #13047 is merged, could you rebase this? I'm hoping the segfault goes away.

rebased

Buildkite fails on this dmd PR: dlang/dmd#12890 ``` ./submodules/ocean/src/ocean/core/SmartUnion.d(549): Warning: calling `ocean.core.SmartUnion.SmartUnion!(HasDuplicates).SmartUnion.opCall` without side effects discards return value of type `SmartUnion!(HasDuplicates)`; prepend a `cast(void)` if intentional ```

dkorpel · 2021-11-16T15:17:37Z

src/dmd/json.d

@@ -302,8 +302,7 @@ public:
            //property(name, "impure");
            break;
        case PURE.weak:     return property(name, "weak");
-        case PURE.const_:   return property(name, "const");
-        case PURE.strong:   return property(name, "strong");
+        case PURE.const_:   return property(name, "strong");


Is this mismatch deliberate?

Is this mismatch deliberate?

You mean "const_" vs "strong"? I can rename the enum if you like. I just kept it as "PURE.const_" to make sure that I'm not missing "PURE.strong" being used with the old meaning.

I think PURE.strong is a better name, but I'm fine with this as well for this PR, it can easily be renamed later. I just wanted to make sure this wasn't a mistake.

dkorpel · 2021-11-16T15:18:28Z

src/dmd/mtype.d

+            return 1;
+    }
+
+    /* The rest of this is too strict; fix later.


Indentation of this comment (and others) is off

Indentation of this comment (and others) is off

fixed

dkorpel · 2021-11-16T15:18:45Z

src/dmd/mtype.d

+ * Determine mutability of indirections in (ref) t.
+ *
+ * Returns: When the type has any mutable indirections, returns 0.
+ * When all indirections are immutable, returns 2.


Missing a Params: section

Missing a Params: section

fixed

Buildkite fails on this dmd PR: dlang/dmd#12890 ``` ./submodules/ocean/src/ocean/core/SmartUnion.d(549): Warning: calling `ocean.core.SmartUnion.SmartUnion!(HasDuplicates).SmartUnion.opCall` without side effects discards return value of type `SmartUnion!(HasDuplicates)`; prepend a `cast(void)` if intentional ```

dkorpel · 2021-11-23T12:14:49Z

The PR for 'ocean' is merged, a rebase should make buildkite green now.

aG0aep6G · 2021-11-23T16:05:51Z

The PR for 'ocean' is merged, a rebase should make buildkite green now.

rebased

Geod24 · 2021-12-12T09:51:51Z

What's the plan here ? I personally don't like this warning, because, like every warnings, it can have a high number of false positives. Did it find any issue(s) in the project that are tested ?

Issue 22130 - [REG2.080.1][DIP1000] pure factory functions stopped working Issue 17635 - [REG 2.066.0] cannot convert unique immutable(int)** to immutable Immutable parameters are not needed for strong purity. Const is enough. Mutability in the return type is an additional condition on top of strong purity when considering side effects. The test case for issue 17635 was inverted without good reason in <dlang#8048>. Flipping again to revert that mistake.

like it used to be

aG0aep6G · 2021-12-12T12:26:36Z

What's the plan here ? I personally don't like this warning, because, like every warnings, it can have a high number of false positives. Did it find any issue(s) in the project that are tested ?

My plan is to fix issues 22130 and 17635. I don't care about the warning.

dkorpel · 2021-12-12T17:51:44Z

I personally don't like this warning, because, like every warnings, it can have a high number of false positives.

The warning was always there, this PR just makes it more accurate. It's similar to when exit() was changed to be noreturn, spawning new 'unreachable code' warnings in e.g. case X: exit(1); break;. It's really annoying when buildkite projects treat warnings as errors, because that makes improvements to dmd that affect warnings a breaking change. I wish dub wouldn't treat warnings as errors by default.

dkorpel · 2022-01-24T11:00:54Z

@RazvanN7 Can you help advance this PR? It's blocked by buildkite because swarm and turtle need to use the newest version of ocean (see sociomantic-tsunami/ocean#857), but I can't get any answer from anyone at sociomantic on GitHub or Slack and I've been trying for weeks now.

I also thought maybe the buildkite configuration could be changed to not treat warnings as errors, but I don't see buildkite configuration files in this repository, so it seems I need a buildkite account with access before I can look at that.

MoonlightSentinel · 2022-01-24T11:22:36Z

I also thought maybe the buildkite configuration could be changed to not treat warnings as errors, but I don't see buildkite configuration files in this repository, so it seems I need a buildkite account with access before I can look at that.

The BuildKite configuration is stored in the CI repo, i.e. the buildkite directory

RazvanN7 · 2022-01-24T11:29:32Z

@dkorpel I have privately contacted @Geod24 to ask for some pointers on how we can move on with updating turtle and swarm. I am waiting for his response. Not treating warnings as errors is a viable alternative in my opinion.

RazvanN7 · 2022-01-25T09:29:31Z

@dkorpel I restarted the buildkite pipeline and toggled auto-merge on. However, a rebase might be necessary to pick-up the latest version of buildkite. I hope not, but let's see.

Thanks @aG0aep6G and @dkorpel for seeing this through.

dlang-bot added the Severity:Bug Fix label Jul 19, 2021

aG0aep6G force-pushed the 22130 branch from 9936e22 to a6125e0 Compare July 19, 2021 10:57

aG0aep6G requested a review from ibuclaw as a code owner July 19, 2021 10:57

thewilsonator reviewed Jul 19, 2021

View reviewed changes

src/dmd/frontend.h Show resolved Hide resolved

src/dmd/mtype.d Show resolved Hide resolved

This was referenced Jul 19, 2021

add cast(void) to silence warnings in tests dlang/druntime#3521

Merged

add cast(void) to silence warnings in tests dlang/phobos#8169

Merged

dlang-bot added the Review:Needs Work label Jul 19, 2021

aG0aep6G force-pushed the 22130 branch from 830f4e5 to 31f6fe8 Compare July 21, 2021 05:14

dlang-bot removed the Review:Needs Work label Jul 21, 2021

aG0aep6G force-pushed the 22130 branch from e11da86 to bd82f6e Compare July 21, 2021 18:53

thewilsonator force-pushed the 22130 branch from bd82f6e to 0bf5bcc Compare August 5, 2021 02:05

dlang-bot added the Review:Needs Work label Oct 1, 2021

dlang-bot added the Review:stalled label Oct 20, 2021

aG0aep6G force-pushed the 22130 branch from 0bf5bcc to b3b725f Compare November 14, 2021 11:35

dlang-bot removed Review:Needs Work Review:stalled labels Nov 14, 2021

dkorpel mentioned this pull request Nov 14, 2021

Explicitly discard SmartUnion in unittest sociomantic-tsunami/ocean#852

Merged

RazvanN7 closed this Nov 16, 2021

RazvanN7 reopened this Nov 16, 2021

dkorpel reviewed Nov 16, 2021

View reviewed changes

aG0aep6G force-pushed the 22130 branch from 57997e9 to e1df298 Compare November 23, 2021 16:05

dkorpel mentioned this pull request Dec 11, 2021

New release needed to unblock dmd's buildkite sociomantic-tsunami/ocean#857

Closed

aG0aep6G added 8 commits December 12, 2021 12:42

remove unused label

3732094

remove PURE.strong from mtype.h

65c10f8

use Mem.check

f865abf

fixup! use Mem.check

967c2a7

require immutable indirections for callSideEffectLevel to return 2

69ed7ce

like it used to be

indentation

10c5bb8

Params

5945612

aG0aep6G force-pushed the 22130 branch from e1df298 to 5945612 Compare December 12, 2021 11:42

RazvanN7 approved these changes Jan 25, 2022

View reviewed changes

RazvanN7 added the Merge:auto-merge-squash label Jan 25, 2022

RazvanN7 merged commit 0dbe44f into dlang:master Jan 25, 2022

dkorpel mentioned this pull request Jan 25, 2022

Fix issue 22139 - Compiler special cases object.dup when compiling with -preview=dip1000 #13571

Merged

aG0aep6G deleted the 22130 branch February 11, 2022 13:13

dkorpel mentioned this pull request Feb 25, 2022

Add std.traits.isStronglyPure dlang/phobos#6403

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix issues 22130, 17635 - pure factory functions #12890

fix issues 22130, 17635 - pure factory functions #12890

aG0aep6G commented Jul 19, 2021

dlang-bot commented Jul 19, 2021 •

edited

Loading

thewilsonator left a comment

thewilsonator commented Jul 19, 2021

thewilsonator commented Jul 19, 2021

aG0aep6G commented Jul 19, 2021

thewilsonator commented Jul 20, 2021

aG0aep6G commented Jul 20, 2021

thewilsonator commented Jul 20, 2021

aG0aep6G commented Jul 21, 2021

dkorpel commented Nov 13, 2021

aG0aep6G commented Nov 14, 2021

dkorpel Nov 16, 2021

aG0aep6G Nov 16, 2021

dkorpel Nov 23, 2021

dkorpel Nov 16, 2021

aG0aep6G Nov 16, 2021

dkorpel Nov 16, 2021 •

edited

Loading

aG0aep6G Nov 16, 2021

dkorpel commented Nov 23, 2021

aG0aep6G commented Nov 23, 2021

Geod24 commented Dec 12, 2021

aG0aep6G commented Dec 12, 2021

dkorpel commented Dec 12, 2021 •

edited

Loading

dkorpel commented Jan 24, 2022 •

edited

Loading

MoonlightSentinel commented Jan 24, 2022

RazvanN7 commented Jan 24, 2022

RazvanN7 commented Jan 25, 2022

fix issues 22130, 17635 - pure factory functions #12890

fix issues 22130, 17635 - pure factory functions #12890

Conversation

aG0aep6G commented Jul 19, 2021

dlang-bot commented Jul 19, 2021 • edited Loading

Bugzilla references

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

Testing this PR locally

thewilsonator left a comment

Choose a reason for hiding this comment

thewilsonator commented Jul 19, 2021

thewilsonator commented Jul 19, 2021

aG0aep6G commented Jul 19, 2021

thewilsonator commented Jul 20, 2021

aG0aep6G commented Jul 20, 2021

thewilsonator commented Jul 20, 2021

aG0aep6G commented Jul 21, 2021

dkorpel commented Nov 13, 2021

aG0aep6G commented Nov 14, 2021

dkorpel Nov 16, 2021

Choose a reason for hiding this comment

aG0aep6G Nov 16, 2021

Choose a reason for hiding this comment

dkorpel Nov 23, 2021

Choose a reason for hiding this comment

dkorpel Nov 16, 2021

Choose a reason for hiding this comment

aG0aep6G Nov 16, 2021

Choose a reason for hiding this comment

dkorpel Nov 16, 2021 • edited Loading

Choose a reason for hiding this comment

aG0aep6G Nov 16, 2021

Choose a reason for hiding this comment

dkorpel commented Nov 23, 2021

aG0aep6G commented Nov 23, 2021

Geod24 commented Dec 12, 2021

aG0aep6G commented Dec 12, 2021

dkorpel commented Dec 12, 2021 • edited Loading

dkorpel commented Jan 24, 2022 • edited Loading

MoonlightSentinel commented Jan 24, 2022

RazvanN7 commented Jan 24, 2022

RazvanN7 commented Jan 25, 2022

dlang-bot commented Jul 19, 2021 •

edited

Loading

dkorpel Nov 16, 2021 •

edited

Loading

dkorpel commented Dec 12, 2021 •

edited

Loading

dkorpel commented Jan 24, 2022 •

edited

Loading