ts.createEmitHostFromProgram doesn't bind compilerHost methods to the compilerHost instance

[Arnavion]

Starting from 5a2fb94, the EmitHost returned from ts.createEmitHostFromProgram() returns functions on the compilerHost like so:

        return {
            getCanonicalFileName: compilerHost.getCanonicalFileName,
            getCurrentDirectory: compilerHost.getCurrentDirectory,
            getNewLine: compilerHost.getNewLine,
            writeFile: compilerHost.writeFile,
        };

Because of this, the implementations of those functions do not have access to the compilerHost object via this.

This doesn't matter for the default WScript and node.js compiler hosts because they use closures to capture their references, but it's a problem for custom compiler hosts that are implemented as classes, and worked before this change.

[RyanCavanaugh]

I don't think we want to take the commit referenced this as it introduces an extra level of function call indirection that isn't always necessary. It's fine to just have the classes themselves use arrow functions instead of prototype methods when they implement compiler hosts.

[Arnavion]

From an implementer's point of view, there is no way to tell from an interface definition that the implementation functions won't be called with a valid this context. Interfaces in TS don't have a way of specifying whether their members needs a valid this context or not.

On the other hand, for a caller of an interface, providing them a valid context is always going to work - it might be really necessary (when the implementation is a class) or it might be ignored (when the implementation is a closure), but not providing a valid context violates the principle of least surprise. One can write a class FooImpl that implements FooInterface correctly according to the compiler, but fails at runtime.

Of course, you could document that "All implementations of the interfaces in this library should expect to have their methods called without a valid this context," but this only increases the cognitive burden of the implementer. This pattern is also harder to implement with classes - the interface has to be implemented twice, once on the class and once inside a new asFooInterface(): FooInterface method. Specifically:

class MyCompilerHost {
    getSourceFile(filename: string, languageVersion: ScriptTarget, onError?: (message: string) => void): ts.SourceFile {
        ...
    }

    asCompilerHost(): CompilerHost {
        // Bind this
        return {
            getSourceFile: (filename, languageVersion, onError) => this.getSourceFile(filename, languageVersion, onError),
            ...
        };
    }
}

...

var myCompilerHost = new MyCompilerHost();
ts.createProgram(myCompilerHost.asCompilerHost());

And then you have to document to use myCompilerHost.asCompilerHost() when interfacing with the TS API - accidentally using myCompilerHost directly will not be an error even though it doesn't have an implements ts.CompilerHost clause because of structural typing!

ts.createProgram(myCompilerHost); // Compiles, but wrong!

[danquirk]

You're describing a fairly general problem with APIs of this shape are you not? The compiler certainly does this in more places than just this one.

[Arnavion]

You're describing a fairly general problem with APIs of this shape are you not?

Yes, which is why I advocated for the safe path of always passing in a valid this-context and letting the implementation ignore it if it wants to. Unless TS gains a syntax like interface FooInterface { bar(this: undefined, arg1: string, arg2: number); } it is not possible for an implementation to know whether their FooImpl.method code can use this or not. If such a syntax existed and the compiler disallowed the use of this inside FooImpl.method then it'd be fine.

So yes, I consider it a problem that the compiler allows two ways of implementing an interface without type errors, yet one of them fails at runtime because of a reason that the type system does not express.

The compiler certainly does this in more places than just this one.

CompilerHost is the one I implement in my compiler wrapper, so I know it broke with the mentioned change. In the PR for this, @DanielRosenwasser suggested to give the Program object the same treatment because users might want to pass in custom Programs not created via ts.createProgram, but I haven't checked to see if any other code that uses Program objects has the issue. Atleast for compiling via ts.createProgram() with a custom CompilerHost, this fix is sufficient.

If there are more places that support user implementations of the interfaces but don't pass in a valid this-context to their methods then they need to be fixed too.

[danquirk]

Perhaps this is something that should be considered along with the other functionality mentioned in issues like #229 and #285

[Arnavion]

Yes, those issues are what I was referring to.

[basarat]

I haven't looked at the code, but would this fix it:

return {
            getCanonicalFileName: compilerHost.getCanonicalFileName.bind(compilerHost),
            getCurrentDirectory: compilerHost.getCurrentDirectory.bind(compilerHost),
            getNewLine: compilerHost.getNewLine.bind(compilerHost),
            writeFile: compilerHost.writeFile.bind(compilerHost),
        };

I think this is what you are suggesting here. If so, I don't see why it can't be done?

[Arnavion]

@basarat Pretty much, although I prefer it to be explicit rather than use bind, seeing as bind kills typechecking - see #1546

[Arnavion]

@mhegazy I think you misunderstood something. This isn't fixed.

[danquirk]

Looks like the referenced PR fixed some but not all of these:

function getEmitHost(writeFileCallback?: WriteFileCallback): EmitHost {
            return {
                getCanonicalFileName: fileName => host.getCanonicalFileName(fileName),
                getCommonSourceDirectory: program.getCommonSourceDirectory,
                getCompilerOptions: program.getCompilerOptions,
                getCurrentDirectory: () => host.getCurrentDirectory(),
                getNewLine: () => host.getNewLine(),
                getSourceFile: program.getSourceFile,
                getSourceFiles: program.getSourceFiles,
                writeFile: writeFileCallback || (
                    (fileName, data, writeByteOrderMark, onError) => host.writeFile(fileName, data, writeByteOrderMark, onError)),
            };
        }

[mhegazy]

@danquirk which ones are still missing? i see all the host references handled correctelly

[mhegazy]

I believe this is fixed now after @Arnavion 's last change. @Arnavion can you confirm?

[Arnavion]

I'll confirm when I get home tonight.

The reason I didn't fix the program ones is in this comment in the PR.