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

use a TypeContext instead of extending the ParserState #6

Conversation

nissy-dev
Copy link
Owner

Summary

Test Plan

Documentation

  • The PR requires documentation
  • I will create a new PR to update the documentation

Comment on lines +264 to +268
parse_ts_type(p, context.and_conditional_type_allowed(true))
.or_add_diagnostic(p, expected_ts_type);
p.expect(T![:]);
parse_ts_type(p).or_add_diagnostic(p, expected_ts_type);
parse_ts_type(p, context.and_conditional_type_allowed(true))
.or_add_diagnostic(p, expected_ts_type);

Choose a reason for hiding this comment

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

Nit: conditional_type_allowed is guaranteed to be true because of the is_conditonal_type_allowed check above.

@@ -640,7 +675,7 @@ fn parse_ts_mapped_type(p: &mut JsParser) -> ParsedSyntax {
p.expect(T!['[']);
parse_ts_type_parameter_name(p).or_add_diagnostic(p, expected_ts_type_parameter);
p.expect(T![in]);
parse_ts_type(p).or_add_diagnostic(p, expected_ts_type);
parse_ts_type(p, TypeContext::default()).or_add_diagnostic(p, expected_ts_type);

Choose a reason for hiding this comment

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

TypeContext::default should only used in positions that start a new "type-parsing" context (e.g. because it is parenthesized or because it is in an initializer position). It's otherwise best to pass the context to all parse_ts_type functions

Suggested change
parse_ts_type(p, TypeContext::default()).or_add_diagnostic(p, expected_ts_type);
parse_ts_type(p, context).or_add_diagnostic(p, expected_ts_type);

The same applies to many other positions (e.g. mapped types, tuple types etc)

Copy link
Owner Author

@nissy-dev nissy-dev Dec 29, 2022

Choose a reason for hiding this comment

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

@MichaReiser

The only thing that we need to be careful about is to make sure to pass context through correctly and only use TypeContext::default in positions where a type starts a complete new context (because it is parenthesized)

It is really hard for me to pass context through correctly. I tried, but I couldn't do that. There are too many functions which the context should be passed to.

@nissy-dev nissy-dev closed this Dec 31, 2022
@nissy-dev nissy-dev deleted the support-extends-constraints-on-infer-type-refactor branch January 8, 2023 00:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants