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

Proposal: Ignore private/protected members when doing inferrence #11907

Closed
chrisnicola opened this issue Oct 27, 2016 · 8 comments
Closed

Proposal: Ignore private/protected members when doing inferrence #11907

chrisnicola opened this issue Oct 27, 2016 · 8 comments
Labels
Needs More Info The issue still hasn't been fully clarified

Comments

@chrisnicola
Copy link

TypeScript Version: 2.0.3 / nightly (2.1.0-dev.201xxxxx)

Code

A trivial example of the problem I'm referring to is in the Gist linked below.

https://gist.github.com/chrisnicola/918f1f0cf3ddf35df2a354514edbf15a

Expected behavior:

I actually expected that this already worked this way, but I was surprised to find out that twoj types are incompatible even if they differ in their private members, not just their public members. I'm not sure if there are valid issues with inferring two objects of different types to be compatible based solely on their public methods but I can't really see why they wouldn't be.

The lack of compatibility here leads to just declaring the object as any to get around the compiler error, which feels like a suboptimal solution.

Actual behavior:

Two types are only compatible if the requested type is a subset of the provided type inclusive of all the requested types private members. My expectation would be that only public members are actually considered.

Resoning:

There are many cases where inference is preferred to declaring and using interfaces or using inheritance. Test substitutes are one example. We often use concrete test substitutes rather than mocking or spying libraries in our tests. These substitutes have identical public interfaces to the concrete implementations but are, obviously, of a different type.

While declaring a common interface would be one solution, as we are testing services in Angular 2 interfaces are an inconvenience with Angular 2's @Injectable behaviour. Also I personally feel it should be unnecessary, it is again just extra ceremony and given the choice I would just stick with just declaring the substitute instance as any in the test instead.

@chrisnicola chrisnicola changed the title Ignore private/protected members when doing inferrence Proposal: Ignore private/protected members when doing inferrence Oct 27, 2016
@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Oct 27, 2016

This seems like a bad idea?

class Alpha {
  x: number = 0;
  private y: string = "me";
  equals(other: Alpha) {
    return this.y.toLowerCase() === other.y.toLowerCase();
  }
}

class Beta {
  x: number = 0;
  private y: number = 0;
  equals(other: Alpha) { return false; }
}

let a = new Alpha();
let b = new Beta();
a.equals(b); // OK according to types, but throws

@chrisnicola
Copy link
Author

chrisnicola commented Oct 28, 2016

Yeah we discussed this problem amongst our group and I agree there are more edge cases than I originally considered. That being said I feel like there may be ways for the compiler to manage this while providing the usability/productivity benefit of not always needing to define a common interface class just to make this work.

In the case above this is only possible because you've passed an object of a type to another object of the same type. In that case could the compiler not flag that as a potential error, or even be so specific as to warn based on the fact that you have an actual usage of a private variable.

In other words, could the compiler, as a default, infer that a type is represented by it's public interface, but if in usage a part of the private interface is accessed, then it would infer it as being represented by its whole?

Another way might be some sort of annotation or way of using a type that implies you intend only its public interface.

My main motivation is that 90% of all usages involve only the public interface.

@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. labels Oct 28, 2016
@appsforartists
Copy link

Would this make sense as a generic type, e.g. Public<Alpha> or ShapeOf<Alpha>, that would give you an interface that includes only the public aspects of Alpha?

In midicast, I've got a Lerna-esque monorepo. If I let each package have its own copy of RxJS, I get errors to the effect of "Observable is not compatible with Observable because they have different protected implementations of source". If I hoist the rxjs dependency to a single node_modules folder that is shared amongst them all, TypeScript stops finding types in sibling packages, and starts treating them as any.

@mhegazy
Copy link
Contributor

mhegazy commented May 31, 2017

If I hoist the rxjs dependency to a single node_modules folder that is shared amongst them all, TypeScript stops finding types in sibling packages, and starts treating them as any.

do you have path mapping entry for rxjs? can you share a repro?

@appsforartists
Copy link

Try:

git clone https://github.com/appsforartists/midicast.git
lerna run build
lerna bootstrap

and look at the type of messages in SongScanner.

@mhegazy
Copy link
Contributor

mhegazy commented Nov 23, 2017

I believe this is now addressed by #6496, can you give typescript@2.6 a try and let us know if you are still running into issues?

@mhegazy mhegazy added Needs More Info The issue still hasn't been fully clarified and removed Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript labels Nov 23, 2017
@appsforartists
Copy link

Worked. Thanks.

@typescript-bot
Copy link
Collaborator

Automatically closing this issue for housekeeping purposes. The issue labels indicate that it is unactionable at the moment or has already been addressed.

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Needs More Info The issue still hasn't been fully clarified
Projects
None yet
Development

No branches or pull requests

5 participants