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

Member Prediction wrong in Typescript Code.... #46084

Closed
jogibear9988 opened this issue Sep 27, 2021 · 8 comments
Closed

Member Prediction wrong in Typescript Code.... #46084

jogibear9988 opened this issue Sep 27, 2021 · 8 comments
Labels
Duplicate An existing issue was already created

Comments

@jogibear9988
Copy link

jogibear9988 commented Sep 27, 2021

Bug Report

Typescript wrongly returns:
This condition will always return 'false' since the types 'ParserState.ParseBinary' and 'ParserState.ParseMember' have no overlap.

🔎 Search Terms

member prediction wrong

🕗 Version & Regression Information

Don't know if it is a regression. Does not work with current Version: 4.4.3

⏯ Playground Link

https://www.typescriptlang.org/play?#code/KYDwDg9gTgLgBMAdgVwLZwAoEMoGdhQDKMWMwcA3gFBy1wByEiwANDXdnsAEICWiOAJ5s6mHPgCywVACMCIjuJ78hjKbPlUAvlSqhIsOAGMANlly44AUXBRgF3kwAqEAGK8TZKFZPSkMTnwoWmoqAEgwKF4AN1JyXBIyAC4xLiJE8gBeVKDiOIA6RmYAbl1RSIgyIzIAEzho3lxeGAAKAEpKdlFaGAALRvyEuLhswII8snyx9TkoUtEdcIqq2vrG5r4BKEF2zrCwvoGhshGc8YyppU2hUv3D3HyGpta28P233gAzOBb7wYyRqMlOkCtNpLNXvsDv0HscsmcQZMxtdtmpwQRbjotEA

💻 Code

	export enum ParserState {
		None,
		ParseBinary,
		ParseMember,
		ParseBinaryNoMember,
	}

	export class ExpressionToFilterElementParser  {

		private state: ParserState = ParserState.None;

		protected visit() {
			this.state = ParserState.ParseMember;
		}

		protected visitBinary() {
			this.state = ParserState.ParseBinary;
			this.visit()
				
			if (this.state == ParserState.ParseMember)
				this.state = ParserState.ParseBinaryNoMember;
		}
	}

🙁 Actual behavior

Typescript complains line

    if (this.state == ParserState.ParseMember)

with error:
This condition will always return 'false' since the types 'ParserState.ParseBinary' and 'ParserState.ParseMember' have no overlap.

🙂 Expected behavior

Code should work

@fatcerberus
Copy link

le sigh
#9998

@MartinJohns
Copy link
Contributor

MartinJohns commented Sep 27, 2021

#44336 (comment)

You know better than not to fill out the issue template. 🤨

@jogibear9988
Copy link
Author

I will fix the issue in the evening, sorry

@jogibear9988
Copy link
Author

jogibear9988 commented Sep 28, 2021

@MartinJohns
Sorry, for doing it wrong (again). Hopefully it's better so

@jogibear9988
Copy link
Author

When I see how mayn issues there are linked in #9998, wouldn't it be better to disable the FlowAnalysis (at least after function calls wich could modify state) as default, and enable it via setting?
Or introduce a way to mark functions if they have side effects or not.

@fatcerberus
Copy link

fatcerberus commented Sep 28, 2021

#9998 itself explains why that would be a bad idea: TS would have to assume every single function call modified your local state and reset all your narrowings, making CFA practically useless.

As for the side effect annotation, see #7770.

The number of issues linked to #9998 is simply because there’s no easy way to solve it, or else it would have been solved already.

@jogibear9988
Copy link
Author

i think it's mostly useless since in classes most methods would modify the state of a class variable. If not, it does not need to be a method of the class? (if it is a function, then this maybe could be different, but i would not asume so)

@andrewbranch andrewbranch added the Duplicate An existing issue was already created label Oct 1, 2021
@typescript-bot
Copy link
Collaborator

This issue has been marked as a 'Duplicate' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

5 participants