pack.NonMainModuleClass access works when module is imported

[nadako]

The following code currently works, however I am not sure if it's supposed to, because (as far as I understand) import statement should only affect unqualified identifier resolution, and NOT fully-qualified (i.e. A and pack.Module.A should resolve, but pack.A should NOT).

Main.hx

import pack.Module;

class Main {
  static function main() {
    trace(pack.A);
  }
}

pack/Module.hx

package pack;

class A {}

[Simn]

Note that even if we deem this a bug, I don't think we should break it in a dot release. It doesn't do any harm because there can only be at most one pack.A type, which means that there's no risk of bad resolution.

[RealyUniqueName]

I don't see any value in changing this.

[nadako]

Well, it's surely minor, but IMO the relation between packages, modules and types in our system is confusing enough already and this inconsistency is not helping.

[kLabz]

This could "break" some macros that don't handle correctly name + sub but currently work because of pack.A resolution. Not a big deal but this may indeed not be worth breaking/fixing in a dot release.

[nadako]

Well, arguably, these macros are already broken since they depend on whether the module is imported at the usage place or not. And IMO this fact favors fixing this, because then people will write proper macros :)
But anyway, if by dot release you mean patch release then I agree it's not worth it. I would probably fix this in a minor release though...

[uvtc]

So I understand this correctly, there are two cases:

1. when you don't import the module
2. when you do

? (Also given the fact that Haxe affords me the shorthand to specify class names like "pack.Module" instead of the more strictly accurate "pack.Module.Module" for a class Module in module pack/Module.hx.)

As it stands now (with Haxe v4.0.5), given pack/Module.hx:

package pack;

class Module {}

class A {}

Case 1: when you don't do the import:

in Main.hx:

//import pack.Module;

class Main {
    public static function main() {

        trace(pack.Module.Module);
        trace(pack.Module); // The allowed shorthand.
        //trace(Module);    //<--- ERROR

        trace(pack.Module.A);
        //trace(pack.A);    //<--- ERROR
        //trace(Module.A);  //<--- ERROR
        //trace(A);         //<--- ERROR
    }
}

Seems straightforward and reasonable.

Case 2: when you do do the import:

import pack.Module;

class Main {
    public static function main() {

        trace(pack.Module.Module);
        trace(pack.Module); // The allowed shorthand.
        trace(Module);      // Even shorter shorthand.

        trace(pack.Module.A);
        trace(pack.A);     // (1)
        //trace(Module.A); //<--- ERROR
        trace(A);          // (2)
    }
}

So, the only thing up for consideration here is removing the one marked (1), right? Or (2) as well?

The one marked (2) strikes me as strange though, because if I have a bunch of modules being imported, how would I know where A came from?

Is there a strict mode that would disallow (2)?

[nadako]

It's about removing (1), yeah.

(2) is easily explainable: all module types are imported in the global namespace, potentially shadowing earlier imports (so import order matters). That said, having a "strict" mode that errors on name clash might not be the worst idea :)

[uvtc]

Thanks, Dan.

Ah, I see. The way (2) works is by design (possibly because it may be useful to shadow type names in that way?).

Yes, being warned if shadowing were to happen seems useful to me. Though, with programming languages I generally tend to want safety nets over the wild west anyway. :)

But my issue with (2) is that there's no easy way to tell from which module A came, other than diving into each import and poking around (or maybe some IDE's do that, I don't know).

[Simn]

I (accidentally) broke this while working on #11168 and took the opportunity to log problems that come from it:

• The test for Cppia overrides the wrong function #5351 breaks
• TestDCE breaks because there's a reference to unit.UsedReferenced2, which is a class that is defined in the same module. I guess this is technically a different case because it's not about imports.
• Docgen fails with Type not found : cs.system.Delegate_AsyncCallback. I don't know what that's about because there's no position.

So in the grand scheme of things, disallowing this kind of resolution should be rather harmless.

[Simn]

Actually the AsyncCallback thing is the same as the UsedReferenced2 thing, and thinking about it we should not disallow that one. The only change we want to make here is to not use this kind of resolution on imports.

Even the heaps samples still compile so this should be fine. Actually they do fail due to a hxsl.TGlobal.

[Simn]

There are two failures in the tink tests:

• typedef MacroOutcomeTools = tink.OutcomeTools This one just looks outright wrong to me because the OutcomeTools type actually lives in tink.core.Outcome. I have no idea why this currently resolves...
• catch (e:haxe.macro.Error) That's a variant of the initial case reported in this issue.

[kLabz]

tink.OutcomeTools is likely the one from https://github.com/haxetink/tink_core/blob/master/src/tink/CoreApi.hx

[Simn]

Ah I see, that makes a bit more sense!