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

fix: Prevent parsing invalid tokens #236

Merged
merged 2 commits into from
Jul 16, 2020
Merged

Conversation

radeksimko
Copy link
Member

@radeksimko radeksimko commented Jul 16, 2020

Fixes #217

The user will often produce invalid/incomplete config as they type, so diagnostics are ignored - which is by design in case of the parser. However the parser also assumes that the sequence of tokens it receives from the lexer is valid, i.e. that the important check for invalid tokens is not ignored.

There are some configs which one might expect to be "parseable", e.g.

provider "null" {
}

variable "name" {
  default = "
}

but the parser isn't ready to deal with such config yet and the lexer returns the trailing } as string literal.

If we wanted to perform completion near the opening quote we would be completing in the context of a string literal. We don't do RHS completion yet anyway, so it is not a big deal to just error out now, but I reckon we will need to revisit this to e.g. support the following

provider "null" {
}

variable "name" {
  default = "${var.
}

We could virtually append closing quote there, to make it a valid sequence, but the question is when (in terms of codepath). If that has to be done before we send bytes to the lexer, then we are practically reimplementing the lexer. We could also just check for particular diagnostics and rerun the lexer - which might be fragile, but it could be guarded by tests, so probably fine.

@radeksimko radeksimko added the bug Something isn't working label Jul 16, 2020
@radeksimko radeksimko force-pushed the b-prevent-parsing-inv-tokens branch 3 times, most recently from fbee26b to 08227a7 Compare July 16, 2020 10:48
@radeksimko radeksimko force-pushed the b-prevent-parsing-inv-tokens branch from 08227a7 to 208c404 Compare July 16, 2020 10:52
Copy link
Contributor

@aeschright aeschright left a comment

Choose a reason for hiding this comment

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

LGTM

@radeksimko radeksimko merged commit dd89629 into master Jul 16, 2020
@radeksimko radeksimko deleted the b-prevent-parsing-inv-tokens branch July 16, 2020 16:38
@ghost
Copy link

ghost commented Aug 15, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the context necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Aug 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Very High CPU consumption
2 participants