New import failure to resolve module locale files with *alias*.

[mhermier]

Hi, if I have something like the following file architecture: main.wren: import "foo" wren_module/foo/foo.wren: import "./bar" wren_module/foo/bar.wren: // Do some really important stuff I get an error like: Could not load module './bar'. but if I modify main.wren: import "foo/foo" Then it works. It seems import aliasing barf oddly there. Cheers

[joshgoebel]

Could you please add a tiny illustration with the layout of where all these files are on your file system or provide a sample Git project that reproduces the issue? Nevermind I see now that I'm looking closely.

[joshgoebel]

FYI: This is because the file imports are resolved one at a time and the path resolution of a parent file doesn't have any effects on the path resolution of files it loads. It only looks at the module name.

IE, with import "foo" the search folder is wren_module/ (so it's looking for bar there). As soon as you change it to foo/foo then the search folder changes to wren_module/foo and bar can be located.

Definitely confusing.

---

This is tricky because both loadModuleFn and resolveModuleFn only take the name of the importer module (foo in your case)... they have no idea the path that it was finally resolved to. So to fix this without changing that API the resolver would need to try all path variants of both the importer and the imported module. IE, search (in no particular order):

• foo/bar.wren
• foo/foo/bar.wren
• for/bar/bar.wren
• for/foo/bar/bar.wren

This will of course have implications for #78 as well.

[joshgoebel]

I'd be happy to take this one if we could agree that the resolver code itself could be written in Wren (not C). I have a test branch already (for native-libraries) where I've done this and it's pretty minimal. I just created a tiny second VM with a single (synchronous) C foreign function that is able to check for the existence of files. No code is loaded into this VM other than the ModuleResolver code (any tiny helpers) - no event loop is necessary, no Fibers involved.

If so it'd be pretty easy for the resolver to resolve this recursively. So when asked to resolve foo imports bar it could re-resolve foo and then use that information to search for bar... that may not be strictly needed, but it wouldn't be difficult.

IE just for an idea:

class Module {
    construct new(importer, module, home) {
        _importer = importer
        _module = module
        _home = home
    }
    static resolve(importer, module, home) {
        var res = Module.new(importer, module, home)
        return res.rootResolver || 
            res.relativeResolver ||
            res.wrenHomeResolver ||
            res.standardResolver 
    }
    rootResolver {
        if (_module.startsWith("/")) return module
    }
    relativeResolver { // ... }
    standardResolver { // ... }
    wrenHomeResolver {
        var locations = [
            Path.join([_home,".wren/lib",_module + ".wren"]),
            Path.join([_home,".wren",_module + ".wren"])
        ]
        for (x in locations) {
            if (File.existsSync(x)) return x
        }        
    }
}

Then of course we could have test cases (also in Wren) to make sure the resolver behavior is fully tested. It probably needs to know the base/cwd directory also, but I was only working on HOME/.wren stuff at time. Anyways I think it's enough to get the idea.

[joshgoebel]

Ok, this is harder than it sounds because you need to change both the resolver and the loader to be smarter. I may have been conflating/mixing them a bit before. The resolver can't instruct the loader properly unless it knows whether you mean foo.wren the file or foo/ (ie, foo/foo.wren the library). It's exactly behavior is to strip the importee to it's nearest directory, then append the imported module so:

• foo => ././bar
• foo/foo => foo/./bar

We should also possibly consider changing the import CLI requirements themself to remove the ambiguity. IE, requiring people to be explicit. IE, make foo/foo the only way it works. Imports should always point to files, not folders. So no more:

  //   // If the module is a single bare name, treat it as a module with the same
  //   // name inside the package. So "foo" means "foo/foo".
  //   if (strchr(module, '/') == NULL) pathJoin(filePath, module);

import "foo/foo" for Foo // loads Foo

import "foo" // Could not load module 'foo'.

[joshgoebel]

If foo/foo is ugly we could also consider:

import "foo/" for Foo // loads Foo from foo/foo

No repeating, AND explicit. Or introduce a new syntax that has this same meaning "I am specifying a library folder, not a file".

[joshgoebel]

I assume you're hacking the CLI though since this stuff doesn't work out of the box?

// TODO: This isn't currently used, but probably will be when package imports
// are supported. If not then, then delete this.
static char* rootDirectory = NULL;
static Path* wrenModulesDirectory = NULL;