Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error on non-ambient class and function merge #32795

Closed
weswigham opened this issue Aug 9, 2019 · 8 comments · Fixed by #44352
Closed

Improve error on non-ambient class and function merge #32795

weswigham opened this issue Aug 9, 2019 · 8 comments · Fixed by #44352
Labels
Domain: Error Messages The issue relates to error messaging Experience Enhancement Noncontroversial enhancements Help Wanted You can do this Suggestion An idea for TypeScript
Milestone

Comments

@weswigham
Copy link
Member

In #32584, we're allowing ambient classes and functions to merge, but non-ambient classes and functions still issue the same Duplicate identifier error. An improved, more specific, error would be useful. Something mentioning that either the function can have an implementation or both need to be ambient.

@weswigham weswigham added Suggestion An idea for TypeScript Help Wanted You can do this Domain: Error Messages The issue relates to error messaging Experience Enhancement Noncontroversial enhancements labels Aug 9, 2019
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Aug 16, 2019
@austincummings
Copy link
Contributor

I can work on this one.

@austincummings
Copy link
Contributor

austincummings commented Sep 13, 2019

@weswigham here's the error message I've come up with. Any thoughts?

Class declaration cannot implement overload list for '{0}'. Function may have implementation or both class and function declarations must be ambient.

@weswigham
Copy link
Member Author

At at glance I'd say that looks good, but I'm wondering if the term ambient is something we've used in an error message yet. @DanielRosenwasser ?

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Sep 13, 2019

At a glance it's fine to say "ambient" as long as you have something actionable.

On the function: Function with bodies can only merge with classes that are ambient.

Related span on the class: Consider adding a 'declare' modifier to this class.

@austincummings
Copy link
Contributor

@DanielRosenwasser would that look something like this then? Or should the "Function with bodies..." messages only appear when hovering over the function name?

image

@weswigham
Copy link
Member Author

Function with bodies can only merge with classes that are ambient. is supposed to be the primary error on the functions, instead of Class declaration cannot implement overload list for '{0}'. Function may have implementation or both class and function declarations must be ambient.. The related spans are both supposed to be referring to the same thing.

@austincummings
Copy link
Contributor

The related spans are both supposed to be referring to the same thing.

@weswigham sorry, I'm a bit confused as to what "same thing" refers to. Here's my understanding.

function Foo(): any;
function Foo(n: number): any {} 
//       ~~~  Function with bodies can only merge with classes that are ambient.
//       No related messages/spans?

class Foo {} 
//    ~~~ Class declaration cannot implement overload list for '{0}'
//    And related "Consider adding 'declare'..." referring to the span of this class?

@weswigham
Copy link
Member Author

The function Foo should have the same related spans as the other error. Otherwise it looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Domain: Error Messages The issue relates to error messaging Experience Enhancement Noncontroversial enhancements Help Wanted You can do this Suggestion An idea for TypeScript
Projects
None yet
4 participants