[WIP - Do not merge] Fix issue 16265 - unittest imports should not be counted for cycles

[schveiguy]

The referenced issue contains the details.

This prevents any dependencies based on the fact that it's in a unittest block. The unit test block shouldn't be called from within a static ctor (normally), so there should be no issue.

However, I have had second thoughts on the situation of instantiated templates with static ctors. In that case, the ctor is placed inside the instantiating module, but the ctor can depend on modules imported by the defining module. In this case, I think we should actually add the imported module as a dependency, but I'm not familiar enough with the dmd code to know how to determine this. I will do some more investigation. The module import should be added (or amended to reset the unittest flag) when the instantiation adds the static ctor to the local module. Don't merge for now.

ping @MartinNowak @WalterBright

[wilzbach]

The referenced issue contains the details.

FYI: Dlang-Bot only picks up references from commits as that's how the changelog between releases is generated.
-> for convenience: https://issues.dlang.org/show_bug.cgi?id=16265

[schveiguy]

@MartinNowak need your input on this. I think I know how to solve the static ctor problem.

What I'm thinking is if we continue the premise that unit tests are effectively not callable from static ctors, then any time a template is instantiated that creates a static ctor/dtor, we can generate a fake module that depends on the instantiating module, and the template defining module, and generate the ctor/dtor in that module. Nothing will depend on it, and so it cannot create cycles. However, it will properly run the ctor before the unit test can be run.

What I don't know is a) how to generate such a fake module, and b) how to make sure it is linked into the binary along with the current module.

[schveiguy]

FYI: Dlang-Bot only picks up references...

I know :) I was pushing this to my branch literally as people were going to dinner after the hackathon, and so didn't have time to properly log the commits. I will fix them when I inevitably rebase.

[JinShil]

@schveiguy Can we get an update on this?

[schveiguy]

@JinShil I don't know if I'm qualified to figure this out. I can play around a bit with the code, and I did learn quite a bit while making this PR, but some of the ideas I have to fix this require an expert at DMD code. And an expert I am not. What do you think of my proposal above?

[JinShil]

@schveiguy I'm sorry, but I'm probably far less skilled than you in this area, believe it or not. I'm seriously wingin' it around here. I just want to be sure the status of each PR is known, and you did that with your response. Thanks!

[ibuclaw]

I don't think you require an extra field to be added to support this logic. Why not use the check:
if (!isUnittest || global.params.useUnittests) as a gate for pushing the module to aimports?

[ibuclaw]

Actually wait, I think this is the wrong angle to attack this from. Shouldn't cyclic dependencies be determined by dependencies on the static constructors themselves?

i.e: This is a cyclic dependency.

// mod1.d:
module mod1;

import mod2;

immutable int val;

shared static this()
{
   val = 2 * multiplier;  // uses mod2.multiplier in ctor
}

// mod2.d:
module mod2;

import mod1;

immutable int multiplier;

shared static this()
{
   multiplier = val;  // uses mod1.val in ctor.
}

[ibuclaw]

I think this patch is just papering over the real problem. But I'd have to dwell on it for a while first.

[schveiguy]

@ibuclaw we can't figure out the true dependencies because we have opaque compilation. If the compiler can't see all the source, how can it trace which variables depend on which other variables?

However, in the case of unit tests, we KNOW that all unit tests a) are run after ctors, and b) aren't (easily) callable from ctors. So any imports they add really shouldn't add dependencies, they can be considered separate compilation units.

But the one place where this breaks down is templates that are instantiated ONLY in the unittests. Those templates maybe trigger some static construction that only they care about, and this means you have to run them locally. They may depend both on static ctors in the current module, and static ctors in the imported template module.

Other than that, I think it's a solid win -- unittests are now used as full examples for documentation, and therefore may have lots of unrelated imports. There's no reason to mess up the dependency cycles based on the unit test imports.

Of course, there certainly are more problems we can solve besides this one. This just struck me as one that happens over and over in Phobos, and is annoying since the real release of phobos isn't affected.

[ibuclaw]

@ibuclaw we can't figure out the true dependencies because we have opaque compilation. If the compiler can't see all the source, how can it trace which variables depend on which other variables?

We don't need transparent compilation, the information visible from the module is good enough.

What I'm saying is:
Currently - we determine cyclic imports at runtime by inferring that somewhere in module A there is an import of B, and somewhere in module B there is an import of A. If both have a static ctor then assume the worst and halt runtime.

Instead - we determine cyclic imports at by inferring that in one of the static ctors in module A there is a use of a symbol imported from B, and when we infer that the same is also true vice versa, halt runtime.

So, rather than treating unittests as a separate compilation unit. I think that static ctors should be separated from the rest of the module instead. We don't need to know exactly where a symbol exists in, only that it is not local and was immediately imported from module "B".

Documenting all import dependencies of static ctors however would require a new module field.

[ibuclaw]

Scanning static ctors only needs to be skin deep as well. The moment we encounter a non-local symbol, we don't need to look any further.

static this()
{
  foo();  // module local function, traverse call.
} 
void foo()
{
  import std.stdio; // import, but not yet used.
  import other;
  bar();  // comes from 'other', add module to ctor dependency list
      // don't scan bar(), we don't need to know what it does.
}

I hope that's clear enough. ☺️

In any case, I don't think what I'm proposing is simple.

[schveiguy]

Thinking this through, I don't think it works.

You can't just consider what static ctors import, because the other modules they depend on might not have static ctors, but might depend on data from external static ctors. In other words, you need the imports from the non-ctor-containing modules as well as the imports from the ones that do contain them.

I think unless you create a graph of the dependencies of all functions, you won't be able to resolve the cycles any better than the crude system we have now.

[schveiguy]

I'm going to close this, as I don't think I'm going to be able to recover what I did. I need to study the compiler code more before coming up with a working solution.