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.

Sign up for GitHub

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

Jump to bottom

fix(language-service): Autocomplete block keywords in more cases #52198

Closed
wants to merge 1 commit into from
Closed

fix(language-service): Autocomplete block keywords in more cases #52198

wants to merge 1 commit into from

Conversation

dylhunn
Copy link
Contributor

@dylhunn dylhunn commented Oct 12, 2023

Previously, autocompletions were not available in two main cases. We correct them.

  1. Autocompletions immediately after @ were usually not working, for example foo @|. We fix this by causing the lexer to not consider the @ part of the text node.
  2. Autocompletions such as @\nfoo, where a newline follows a bare @, were not working because the language service visitor considered us inside the subsequent text node. We fix this by adding a block name span for the block keyword, and special-case whether we are completing inside the name span. If we are, we don't continue to the following text node.

@dylhunn dylhunn added action: review The PR is still awaiting reviews from at least one requested reviewer area: language-service Issues related to Angular's VS Code language service target: rc This PR is targeted for the next release-candidate labels Oct 12, 2023
@ngbot ngbot bot added this to the Backlog milestone Oct 12, 2023
@angular-robot angular-robot bot added the detected: feature PR contains a feature commit label Oct 12, 2023
@dylhunn dylhunn force-pushed the ls-completion-blocks branch 2 times, most recently from 93d3897 to f945cc6 Compare October 13, 2023 01:25
crisbeto
crisbeto approved these changes Oct 13, 2023
View reviewed changes
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

LGTM

packages/compiler/src/ml_parser/lexer.ts Outdated Show resolved Hide resolved
packages/compiler/test/ml_parser/lexer_spec.ts Outdated Show resolved Hide resolved
@atscott
Copy link
Contributor

atscott commented Oct 13, 2023

@dylhunn this should be a fix instead of feat in my opinion

@dylhunn dylhunn removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Oct 16, 2023
@dylhunn dylhunn changed the title feat(language-service): Autocomplete block keywords in more cases fix(language-service): Autocomplete block keywords in more cases Oct 16, 2023
@angular-robot angular-robot bot removed the detected: feature PR contains a feature commit label Oct 16, 2023
@dylhunn dylhunn added the action: presubmit The PR is in need of a google3 presubmit label Oct 17, 2023
@dylhunn dylhunn removed the request for review from atscott October 17, 2023 00:38
@pullapprove pullapprove bot requested a review from atscott October 17, 2023 00:38
@dylhunn
Copy link
Contributor Author

dylhunn commented Oct 17, 2023

@atscott This parser change actually causes some breakage, let's try to sort it out soon. It might require changes to the migration.

@dylhunn
Copy link
Contributor Author

dylhunn commented Oct 17, 2023 •
edited
Loading

@crisbeto The migration has already run in g3, right? Does it depend on the parser, e.g. would running it again cause the remaining @ symbols that are now parsed as incomplete blocks to be picked up?

@crisbeto
Copy link
Member

Yeah, I've run an LSC to migrate all existing usages. Re-running the migration would pick up any new usages since the parser in the migration is set up not to parse blocks, although I'm not sure how many that would be since any new usages would've resulted in compiler errors.

@dylhunn dylhunn added state: blocked and removed action: presubmit The PR is in need of a google3 presubmit labels Oct 17, 2023
@dylhunn
Copy link
Contributor Author

dylhunn commented Oct 17, 2023

Ack. To summarize:

  1. The currently checked-in version of the parser does not ban raw @ followed by a non-word lexeme.
  2. The update in this PR causes these instances of @ to be banned (parsed as incomplete blocks).
  3. Some backsliding of these cases has occurred since the migration was last run.
  4. That means we need to re-run the migration, and then immediately merge this PR.

@dylhunn
Copy link
Contributor Author

dylhunn commented Oct 18, 2023 •
edited
Loading

Running a TGP to find all failures: https://test.corp.google.com/OCL:574331077:BASE:574331065:1697596661631:17c836dd
Edit: tons of failures. I will run tappity to dedup, but it looks like these cases were never picked up, and they're not just backsliding they are special cases that have to be migrated by hand.

@dylhunn
Copy link
Contributor Author

dylhunn commented Oct 19, 2023

Failures in g3 have been cleaned up.

@dylhunn
fix(language-service): Autocomplete block keywords in more cases
f32eb86 
Previously, autocompletions were not available in two main cases. We correct them.

1. Autocompletions immediately after `@` were usually not working, for example `foo @|`. We fix this by causing the lexer to not consider the `@` part of the text node.
2. Autocompletions such as `@\nfoo`, where a newline follows a bare `@`, were not working because the language service visitor considered us inside the subsequent text node. We fix this by adding a block name span for the block keyword, and special-case whether we are completing inside the name span. If we are, we don't continue to the following text node.
@atscott atscott added the action: merge The PR is ready for merge by the caretaker label Oct 20, 2023
@ngbot
Copy link

ngbot bot commented Oct 20, 2023

I see that you just added the action: merge label, but the following checks are still failing:
    failure status "google-internal-tests" is failing
    pending status "pullapprove" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken main, please try rebasing to main and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@dylhunn
Copy link
Contributor Author

dylhunn commented Oct 20, 2023 •
edited
Loading

Rerunning failures, now that fixes have landed: https://fusion2.corp.google.com/presubmit/574331077/OCL:574331077:BASE:575308291:1697834805150:9fc015ef/targets
Edit: another re-run: https://fusion2.corp.google.com/presubmit/574331077/OCL:574331077:BASE:575394089:1697866206612:6596faf8/targets
Rerun: https://fusion2.corp.google.com/presubmit/574331077/OCL:574331077:BASE:575868297:1698083795505:7ba5327f/targets

@dylhunn dylhunn removed the action: merge The PR is ready for merge by the caretaker label Oct 20, 2023
@dylhunn
Copy link
Contributor Author

dylhunn commented Oct 23, 2023

Reruns are passing! Ready to merge

@dylhunn dylhunn added the action: merge The PR is ready for merge by the caretaker label Oct 23, 2023
@ngbot
Copy link

ngbot bot commented Oct 23, 2023

I see that you just added the action: merge label, but the following checks are still failing:
    failure status "google-internal-tests" is failing

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken main, please try rebasing to main and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@dylhunn
Copy link
Contributor Author

dylhunn commented Oct 23, 2023

This PR was merged into the repository by commit e6affef.

@dylhunn dylhunn closed this in e6affef Oct 23, 2023
dylhunn added a commit that referenced this pull request Oct 23, 2023
@dylhunn
fix(language-service): Autocomplete block keywords in more cases (#52198
9d565cd 
)

Previously, autocompletions were not available in two main cases. We correct them.

1. Autocompletions immediately after `@` were usually not working, for example `foo @|`. We fix this by causing the lexer to not consider the `@` part of the text node.
2. Autocompletions such as `@\nfoo`, where a newline follows a bare `@`, were not working because the language service visitor considered us inside the subsequent text node. We fix this by adding a block name span for the block keyword, and special-case whether we are completing inside the name span. If we are, we don't continue to the following text node.

PR Close #52198
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 23, 2023
tbondwilkinson pushed a commit to tbondwilkinson/angular that referenced this pull request Dec 6, 2023
@dylhunn @tbondwilkinson
fix(language-service): Autocomplete block keywords in more cases (ang…
d4f5934 
…ular#52198)

Previously, autocompletions were not available in two main cases. We correct them.

1. Autocompletions immediately after `@` were usually not working, for example `foo @|`. We fix this by causing the lexer to not consider the `@` part of the text node.
2. Autocompletions such as `@\nfoo`, where a newline follows a bare `@`, were not working because the language service visitor considered us inside the subsequent text node. We fix this by adding a block name span for the block keyword, and special-case whether we are completing inside the name span. If we are, we don't continue to the following text node.

PR Close angular#52198
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
@dylhunn @ChellappanRajan
fix(language-service): Autocomplete block keywords in more cases (ang…
dcc99a2 
…ular#52198)

Previously, autocompletions were not available in two main cases. We correct them.

1. Autocompletions immediately after `@` were usually not working, for example `foo @|`. We fix this by causing the lexer to not consider the `@` part of the text node.
2. Autocompletions such as `@\nfoo`, where a newline follows a bare `@`, were not working because the language service visitor considered us inside the subsequent text node. We fix this by adding a block name span for the block keyword, and special-case whether we are completing inside the name span. If we are, we don't continue to the following text node.

PR Close angular#52198
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@atscott atscott atscott approved these changes

@crisbeto crisbeto crisbeto approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: language-service Issues related to Angular's VS Code language service target: rc This PR is targeted for the next release-candidate
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@dylhunn @atscott @crisbeto