Refactor object.d: Move destroy and friends to new file, lifetime.d

[JinShil]

object.d has become a dumping ground for anything and everything druntime, and is too unwieldy. For evidence, see the diff at https://github.com/dlang/druntime/pull/2194/files -- it's out of control, and I'm tired of it.

This is a refactoring based on suggestions by @ZombineDev at #2194 (comment)

As suggested by @ZombineDev, my plan is to move related implementations into their own files and publicly import them in object.d. Ideas are currently:
lifetime.d - destroy and friends
array.d - dynamic array support
typeinfo.d - all the TypeInfo classes and maybe even ModuleInfo
associativeArray.d - associative array support
comparison.d - __cmp and __equals
... { you get the idea }

Eventually it would be nice to change object.d to package.d, but I'm getting ahead of myself. For now, let's start with lifetime.d...that's this PR.

There may also be other benefits besides making druntime maintenance more manageable, such as encapsulation. As we submit more PRs to refactor object.d, DMD will also need some changes, but I hope we can visit those when their time comes.

I'm not sure how this will affect the published documentation. I guess we'll soon find out when DAutoTest runs.

[dlang-bot]

Thanks for your pull request, @JinShil!

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.

Testing this PR locally

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

dub fetch digger
dub run digger -- build "master + druntime#2222"

[jacob-carlborg]

Missing newline at end of file.

[wilzbach]

Be a bit careful here, on a PR a couple of months ago Andrei expressed his opinion of not doing the split because he was worried about the performance implications of it.
I doubt that it's even measurable, but we need to show that there's no performance penalty if we want to move forward with this.
FTR I like this proposal too!

[JinShil]

What is the purpose of this sanitizing json test?

Test compilable/json2.d failed.  The logged output:
Executing post-test script: tools/postscript.sh compilable/extra-files/json-postscript.sh compilable json2 generated/e19df33cb94900627d49ff571db157544166b26f48fa87e4767ba86aff44c5c
+ source compilable/extra-files/json-postscript.sh
++ echo SANITIZING JSON...
SANITIZING JSON...
++ generated/sanitize_json generated/compilable/json2.out
++ diff -pu --strip-trailing-cr compilable/extra-files/json2.out generated/compilable/json2.out.sanitized
--- compilable/extra-files/json2.out	2018-06-19 06:05:43.605906305 -0700
+++ generated/compilable/json2.out.sanitized	2018-06-19 06:07:46.682590297 -0700
@@ -62,6 +62,11 @@
                 "name": null
             },
             {
+                "file": "../../druntime/import/lifetime.d",
+                "isRoot": false,
+                "name": "lifetime"
+            },
+            {
                 "file": "compilable/json.d",
                 "isRoot": false,
                 "name": "json"
/home/braddr/sandbox/at-client/pull-3212240-Linux_32_64/dmd/generated/linux/release/32/dmd -conf= -m64 -Icompilable -o- -Xf=generated/compilable/json2.out -Xi=compilerInfo -Xi=buildInfo -Xi=modules -Xi=semantics  -fPIC  -odgenerated/compilable -ofgenerated/compilable/json2_0.o  -c compilable/json2.d 

==============================
Test compilable/json2.d failed: expected rc == 0, exited with rc == 1

[12345swordy]

+1 This makes searching for certain functions much easier.

[wilzbach]

What is the purpose of this sanitizing json test?

It's a wrapper around the json tests that replaces a few fields which are different depending on the host environment, build environment/OS.
In other words, it replaces such fields with VALUE_REMOVED_FOR_TEST.

Have a look at https://github.com/dlang/dmd/blob/3a2d609093c0df1dc008577ad607963fc431fd39/test/tools/sanitize_json.d#L176

It already does filter druntime and phobos out:

if(moduleName.startsWith("std.", "core.", "etc.") || moduleName == "object")

It's just that you introduce a new top-level moduleName that isn't reserved yet.
Can't we rename object.d to object/package.d and put the lifetime module under object/lifetime.d?

[wilzbach]

You can safely set the copyright to the D Language Foundation.

[JinShil]

Can't we rename object.d to object/package.d and put the lifetime module under object/lifetime.d?

DMD is specifically hard-coded to look for object.d, so that will require an update to DMD as well. Also, I'd like to remove the object moniker as object.d really has very little to do with object anymore.

I think I can move lifetime.d to something like internal/lifetime.d for now until we make the necessary DMD changes. We can reshuffle file locations at that time.

[JinShil]

he was worried about the performance implications of it

Please help me understand. How would having these declarations in a separate file and publicly imported negatively affect performance?

[wilzbach]

Please help me understand. How would having these declarations in a separate file and publicly imported negatively affect performance?

He was worried about the extra time needed to open these five more files. We just need to prove that there's no such penalty or that it's not measurable compared to all the other things DMD does.

[wilzbach]

I think I can move lifetime.d to something like internal/lifetime.d for now until we make the necessary DMD changes. We can reshuffle file locations at that time.

You would need to go for core.internal.lifetime or core.object.lifetime as otherwise the json sanitization won't work (it only ignored object, core, etc, and std).

[JinShil]

I went with core.internal.object.lifetime.

[JinShil]

I went back to lifetime.d being a sibling of object.d. I'm going to modify the sanitize json test to make it work.

[wilzbach]

I'm not sure that's a good idea. This will prevent anyone from using lifetime as a top-level file name. What's your motivation?

[JinShil]

My motivation is to model it after other well-designed packages. The one that comes to mind is std.algorithm. https://github.com/dlang/phobos/tree/master/std/algorithm If you look at that file structure there's a package.d and several sibling files that are publicly imported by package.d. Eventually, I'd like to rename object.d to package.d so it follows the same convention. That's my line of thinking.

[wilzbach]

I agree that renaming object.d to object/package.d is a good way to go forward, I'm just worried that we end up with this intermediate step in the next release.

[schveiguy]

@wilzbach is right, object has been used since the beginning so there isn't going to be any conflict as a top-level symbol, but lifetime does not have that same status. Considering that every D program is going to import these modules, they need to not conflict with existing code.

Another thing to think about -- the splitting of this module does not have the same requirements as other package split-ups such as std.algorithm. Every D program imports this module, so if we make object.d import all the sub-modules, then every D program will import all the package submodules. In other words, nobody is every going to import object.lifetime explicitly. So the only benefit to doing this is code organization.

And given that, I don't think it's very important to put these as siblings or a package with object.d in general. Just make core.internal.object package, and have the global object.d import things from there. No compiler changes, no disruption to existing symbol names or build tools, and we just get better code organization.

[JinShil]

I agree that renaming object.d to object/package.d is a good way to go forward,

Actually I was just planning on renaming object.d to package.d in place without moving it to an object subdirectory. object is really a misnomer at this point and when we implicitly import object;, what we really mean in "import druntime", so it makes sense to me for it to be a root package.d file.

Just make core.internal.object package, and have the global object.d import things from there. No compiler changes, no disruption to existing symbol names or build tools, and we just get better code organization.

I would have been ok with that, but it ended up causing strange issues. Take a look at the test failures in https://auto-tester.puremagic.com/show-run.ghtml?projectid=1&runid=3212929&isPull=true

==============================
Test fail_compilation/ice11969.d failed: 
expected:
----
fail_compilation/ice11969.d(9): Error: undefined identifier `index`
fail_compilation/ice11969.d(10): Error: undefined identifier `cond`
fail_compilation/ice11969.d(11): Error: undefined identifier `msg`
----
actual:
----
fail_compilation/ice11969.d(9): Error: undefined identifier `index`
fail_compilation/ice11969.d(10): Error: undefined identifier `cond`, did you mean import `core`?
fail_compilation/ice11969.d(11): Error: undefined identifier `msg`
----

compilable/test8937.d(28): Error: static assert:  `!true` is false

See source code for that test at https://github.com/dlang/dmd/blob/4b8555e9c9b7a840549c68c39eef26782cfbb23f/test/compilable/test8937.d#L26-L28

if (true)
    static import core.stdc.math;
static assert(!__traits(compiles, core.stdc.math.cos(0))); // fails

So, if I go that route, I'm going to be required to make changes to the compiler.

[schveiguy]

Take a look at the test failures

The first failure has to do with the fact that object is importing core into the namespace. Since it now has a symbol levenshtein distance that's low enough to suggest a spelling alternative to cond, the error message is changed. But this does NOT mean that there is a bug in the compiler or requires compiler changes -- you just need to change the expected output. Clearly, the original issue hasn't returned, the compiler is not ICE-ing.

The second issue, that is more interesting. I think this is not just a quirk, but an actual bug in the compiler. You're not importing core.stdc.math but object has imported core.something.else. That shouldn't affect the imports.

I'd say we need to fix that bug regardless of whether we split object.d into pieces. The question is, where does it come from?

But I'd consider having to fix bugs in the compiler preferable to adding/changing features in the compiler. We may have to fix the bugs in either case.

[JinShil]

https://issues.dlang.org/show_bug.cgi?id=19014 is currently blocking progress on this. Any help fixing that would be greatly appreciated.