Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Rule suggestion: strict-string-expressions (no implicit function to string) #4512

Closed
ColCh opened this issue Feb 8, 2019 · 8 comments · Fixed by #4807
Closed

Rule suggestion: strict-string-expressions (no implicit function to string) #4512

ColCh opened this issue Feb 8, 2019 · 8 comments · Fixed by #4807

Comments

@ColCh
Copy link
Contributor

ColCh commented Feb 8, 2019

Rule Suggestion

There is some common pattern

const barFunc = () => 'World';
const fooStr = `Hello, ${barFunc()}`; // Hello, World

But it's easy to miss brackets around function and this would be not an error

const barFunc = () => 'World';
const fooStr = `Hello, ${barFunc}`; // "Hello, () => 'World'"

What does your suggested rule do?

If it's on, require explicit toString or String call around function

Like this

// yeah, we really want function source code here
const barFunc = () => 'World';
const fooStr = `Hello, ${String(barFunc)}`; // "Hello, () => 'World'"

Additional context

What about spreading this rule for other data types?

I think this is frustrating sometimes:

const name = { foo: 'Foo' };
alert('Hello, ' + name); // Hello, [object Object] (╯°□°)╯︵ ┻━┻ 

What do you think?

@ColCh ColCh changed the title new-rule: no function to string new-rule: no implicit function to string Feb 8, 2019
@JoshuaKGoldberg
Copy link
Contributor

@ColCh this is a really interesting idea, thanks! I've definitely felt the exact pain you've described, and would like to see a rule helping.

On the other hand, the following should be allowed, right?

class CanBeStringified {
    toString() {
        return "This works fine...";
    }
}

const stringified = new CanBeStringified();

alert(`Result: ${stringified}`);

So is it that this rule should ban objects that whose toString is "reasonable"? Meaning: primitives and objects/instances with their own toString are fine, but regular methods and functions aren't? What about arrays, unknowns, anys, and type unions or intersections?

Let's discuss a bit to nail down how this should behave.

@roblourens
Copy link

I was just looking for a rule for this. microsoft/vscode#68966

Meaning: primitives and objects/instances with their own toString are fine, but regular methods and functions aren't? What about arrays, unknowns, anys, and type unions or intersections?

Yes, I would want something specifically for functions and methods, (or type unions that could represent functions). I think that's the mistake that I'm most likely to make. It could also be useful for arrays, objects that don't have a non-default toString method, unknown etc.

@ColCh
Copy link
Contributor Author

ColCh commented Mar 4, 2019

Since TSLint migration to ESLint (medium post) and presence of this rule in eslint (no-implicit-coercion), I think it would be nice just to close this issue :) What do you think?

@JoshuaKGoldberg
Copy link
Contributor

@ColCh there are still very many users on TSLint who rely on the features and/or rules that haven't yet been ported over to ESLint. Let's keep this issue open for the time being.

no-implicit-coercion also doesn't quite capture what this rule is proposed to do. Knowing whether something is a function requires type info, so it looks like that rule is a "weaker" form of this one.

@adidahiya
Copy link
Contributor

Let's call this strict-string-expressions, to mirror strict-boolean-expressions

@adidahiya adidahiya changed the title new-rule: no implicit function to string Rule suggestion: strict-string-expressions (no implicit function to string) Mar 12, 2019
@ColCh
Copy link
Contributor Author

ColCh commented Mar 17, 2019

@JoshuaKGoldberg #4512 (comment) That makes things complicated :)

What about this example?

class Greeter {
    constructor(
        private name: string
    ) { }

    getGreetMsg() {
        return `Hello, ${this.name}!`;
    }

    toString() {
        return `Greeter for ${this.name}`;
    }
}

const greeter = new Greeter("World");

alert(`Result: ${greeter}`); // linter -> ERR or OK ?

I think, that in this case .toString() should be called too, like

alert(`Result: ${greeter.toString()}`); // linter -> OK

What about arrays, unknowns, anys, and type unions or intersections

hm, may be all things would require explicit String call but not string's

like

alert('test' + new Greeter()); // ERR
alert('test' + NeverTypeVar); // ERR
alert('test' + []); // ERR
alert('test' + { foo: 'foo' }); // ERR

// e.g. if x not typeof 'string', return ERR status

and pass only for strings

alert('test' + 'FOO'); // OK!

@JoshuaKGoldberg
Copy link
Contributor

@ColCh yes, that makes sense & I agree. The simplest theology here makes sense: complex objects such as class instances and functions should have explicit .toString()s. 👍

@JoshuaKGoldberg
Copy link
Contributor

Note: per #4534, this issue will be closed in less than a month if no PR is sent to add the rule. If you really need the rule, custom rules are always an option and can be maintained outside this repo!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants