[unbound-method] console methods are bound

[ab-pm]

somePromise.catch(console.error);

Expected

The code should be accepted by the linter.

Actual

I'm getting an error, complaining that .error is an unbound method.

I'm not sure how this should be fixed - changing the type declarations of the builtin types to no longer require a this argument? Or providing an exceptions option in the rule configuration?

Versions

package	version
@typescript-eslint/eslint-plugin	2.4.0

[ab-pm]

After looking into this a bit, I guess the proper way to resolve this would be changing types/node global.d.ts (according to the node console definition) to

declare var console: NodeJS.GlobalConsole;

declare namespace NodeJS {
    interface GlobalConsole extends Console {
        Console: NodeJS.ConsoleConstructor;
        log: OmitThisParameter<Console["log"]>;
        error: OmitThisParameter<Console["error"]>;
        // …
    };
}

except that this appears to require coordination with lib.dom.d.ts which also declares a global console of the same type. I have no idea how to properly resolve this.

So probably special-casing the [unbound-method] rule to ignore methods of a Console type is more simple way to achieve my goal.

[G-Rath]

@bradzacher I'm keen to give this a go, but so far all I can come up with is doing something along the lines of

if(
  node.object.name === 'console' && 
  consoleFunctions.includes(node.property.name)
) { 
  return;
}

Which I don't think is the worst solution in the world, but I feel like I should be able to use the type information to get the actual type of console.<x> and compare that - except I can't actually figure out how to get the type 😬

Got any tips or pointers for me?

(Mind you, that solution could actually be considered more error-prone, since unless you can confirm exactly where a type came from, it'd match any interface called Console, instead of any variable :/)

[bradzacher]

@G-Rath, you might be able to do something similar to what I suggested for the no-throw-literal rule:

typescript-eslint/packages/eslint-plugin/src/rules/no-throw-literal.ts

Lines 31 to 40 in 6aa6bc7

	const symbol = type.getSymbol();
	if (symbol?.getName() === 'Error') {
	const declarations = symbol.getDeclarations() ?? [];
	for (const declaration of declarations) {
	const sourceFile = declaration.getSourceFile();
	if (program.isSourceFileDefaultLibrary(sourceFile)) {
	return true;
	}
	}
	}

Though TBH I'm not sure if program.isSourceFileDefaultLibrary will work for this.
If the project includes the dom types (lib.dom.d.ts), then should work fine, but in a NodeJS project which is typed via @types/node, I don't think it will work there.

Maybe it's "good enough" to instead ensure that console isn't defined locally, and that it isn't imported (i.e. console is a global symbol).
NOTE - This will also have a bit of weirdness due to interface/namespace declaration merging.

[G-Rath]

Though TBH I'm not sure if program.isSourceFileDefaultLibrary will work for this.

So close, and yet so far 😭 You're right that it doesn't work unless you include the dom lib, but when it is included... it works like a treat!

I'll have a hunt around and see if I can figure out a cunning way around it, but otherwise I'll look at doing the good enough path 🤷‍♂

[OliverJAsh]

FYI I think there are a few other instances of this problem, e.g. JSON.parse and JSON.stringify.

@ab-pm I'm curious to hear more about how you would solve this by changing the types.

        log: OmitThisParameter<Console["log"]>;

AFAICS, console methods don't specify a this parameter:

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/eea68f0134af6c534f3fd861d9e5e4833447400e/types/node/globals.d.ts#L60

… so why would we need to omit it if it's not there to start with?

[ab-pm]

@OliverJAsh

AFAICS, console methods don't specify a this parameter:

Seems to be implicit because it's declared as a method on the Console interface. Changing the declaration to a property with a function type is one thing that works, using OmitThisParameter is another. (It needs to be done for the global console only though, not for every arbitrary Console instance).

[OliverJAsh]

Seems to be implicit because it's declared as a method on the Console interface.

IIUC, TypeScript does not automatically add this types to class methods: microsoft/TypeScript#35451 (comment). Unless I misunderstand?

When a method does have a this type, TS gives us errors when we try to call the method with the wrong this type:

class Foo {
    bar(this: this) {}
}

declare const foo: Foo;

foo.bar.call(null); // error

But this is not the case for console methods, demonstrating the fact that methods do not have this types:

console.log.call(null); // no error

[ab-pm]

@OliverJAsh You must have more TS knowledge than me then :-) Notice however that I am not using noImplicitThis. Should I?
Here is my experience:

interface Foo {
	bar1(): void;
	bar2(this: this): void;
	bar3: () => void;
}

const foo: Foo = {} as Foo;
const fob: {
	bar1: OmitThisParameter<Foo['bar1']>;
	bar2: OmitThisParameter<Foo['bar2']>;
	bar3: Foo['bar3'];
} = foo;

blah(foo.bar1); // eslint: unbound method
blah(fob.bar1); // ok
blah(foo.bar2); // eslint: unbound method
blah(fob.bar2); // ok
blah(foo.bar3); // ok
blah(fob.bar3); // ok

Only foo.bar2.call(null) causes ts error 2345, the other methods of foo and fob don't complain when being called on null.

[ghost]

Also getting issues for Number.isSafeInteger and Number.parseInt. Maybe these libraries could perhaps mark this: void and this library could take this into account?

Would require type-checking but it would be worth it IMHO.

[G-Rath]

@bradzacher

Maybe it's "good enough" to instead ensure that console isn't defined locally, and that it isn't imported (i.e. console is a global symbol).

Would you have any pointers for this, such as an already existing rule that does this?

Right now I'm going to aim for having something of a hardcoded array we can chuck things like "console", "Number", "String", etc into and then we can think about how to improve things longer term.

[bradzacher]

I would think that you can do something along the lines of like:

1. grab the ts.SourceFile for the current file being linted.
2. grab the declaration(s) for the type (see above comment).
3. ensure all declarations are not in the current source file.

It's not perfect, but I think that it's "good enough".
Having a quick play, I think that this will actually prevent cases like import console from './consoleshim', because I believe this counts as a declaration.