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

Add an extra test case related to return statements and annotated return types #52628

Merged
merged 1 commit into from Mar 16, 2023

Conversation

Andarist
Copy link
Contributor

@Andarist Andarist commented Feb 6, 2023

This is a test case extracted from the "self check". I managed to break it in #52622 without breaking the test suite so I figured out that it's best to get it into the test suite.

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Feb 6, 2023
@typescript-bot
Copy link
Collaborator

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

@jakebailey
Copy link
Member

I noticed this in the test, but I'm honestly a little suspicious of this code because the type variable is only observed once, which makes me think that it's already bound to be broke as it is and maybe should be changed. But, I think that PR had other things going on too.

@Andarist
Copy link
Contributor Author

Andarist commented Feb 6, 2023

Yea, you might be right here - this only works within the context of the contextual type and allowing this is kinda suspicious:

type PropOfRaw<T> = readonly T[] | "not-array" | "no-prop";

declare function isString(text: unknown): text is string;

declare function getPropFromRaw<T>(
  prop: "files" | "include" | "exclude" | "references",
  validateElement: (value: unknown) => boolean,
  elementTypeName: string
): PropOfRaw<T>;

function getSpecsFromRaw(
  prop: "files" | "include" | "exclude"
): PropOfRaw<string> {
  const val = getPropFromRaw(prop, isString, "string");
  return val; // error
}

This code errors even though I only extracted the returned value to the local val variable. It doesn't even change what is actually inferred there - it's PropOfRaw<unknown> in both scenarios, it's just that somehow this is allowed when the value is returned directly. Should I open a new issue about this case? 🤔

@sandersn sandersn added this to Not started in PR Backlog Feb 14, 2023
@sandersn sandersn moved this from Not started to Waiting on reviewers in PR Backlog Feb 14, 2023
Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning back to this, I guess it's okay to take this case, it's just a little weird because the type var is only observed once. But there's clearly no question as to which one it is for this example.

I tried to figure out if we could change this code but I can't quite get it right; the code secretly lies about the type it returns and is totally unsafe.

PR Backlog automation moved this from Waiting on reviewers to Needs merge Mar 16, 2023
@jakebailey jakebailey closed this Mar 16, 2023
PR Backlog automation moved this from Needs merge to Done Mar 16, 2023
@jakebailey jakebailey reopened this Mar 16, 2023
PR Backlog automation moved this from Done to Not started Mar 16, 2023
@jakebailey jakebailey merged commit b8c7168 into microsoft:main Mar 16, 2023
17 checks passed
PR Backlog automation moved this from Not started to Done Mar 16, 2023
@Andarist
Copy link
Contributor Author

Hmm, I took another look at this as I totally forgot about this PR... this code is so... suspicious 😅 Like, it was broken by one of my PRs, and thus might be worthwhile to have it as a test case. I still wonder though if this should actually be permitted. I can't find a reasonable explanation for this:

type PropOfRaw<T> = readonly T[] | "not-array" | "no-prop";

declare function isString(text: unknown): text is string;

declare function getPropFromRaw<T>(
  prop: "files" | "include" | "exclude" | "references",
  validateElement: (value: unknown) => boolean,
  elementTypeName: string
): PropOfRaw<T>;

function getSpecsFromRaw1(
  prop: "files" | "include" | "exclude"
): PropOfRaw<string> {
  const val = getPropFromRaw(prop, isString, "string");
  return val; // error
}

function getSpecsFromRaw2(
  prop: "files" | "include" | "exclude"
): PropOfRaw<string> {
  return getPropFromRaw(prop, isString, "string"); // ok?
}

Dunno, perhaps I will open a new issue to continue discussing this 🤷‍♂️

@Andarist
Copy link
Contributor Author

After second thought... maybe this is just valid "transitively"? While this code is suspicious on its own... we can't write the implementation for getPropFromRaw that wouldn't error. Since we don't have any access to T in it (as it doesn't appear in the parameters list), we also can't return anything that would be assignable to it (without casting).

@Andarist Andarist deleted the test/contextual-return-type branch March 16, 2023 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
PR Backlog
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants