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

Merge main to tokenizer branch #10872

Merged

Conversation

333fred
Copy link
Member

@333fred 333fred commented Sep 10, 2024

Will call out the merge locations.

dotnet-maestro bot and others added 30 commits August 12, 2024 12:42
…ence-packages build 20240805.2

Microsoft.SourceBuild.Intermediate.source-build-reference-packages
 From Version 9.0.0-alpha.1.24374.1 -> To Version 9.0.0-alpha.1.24405.2
…812.1

Microsoft.SourceBuild.Intermediate.arcade , Microsoft.DotNet.Arcade.Sdk
 From Version 8.0.0-beta.24376.1 -> To Version 8.0.0-beta.24412.1
…814.3

Microsoft.SourceBuild.Intermediate.arcade , Microsoft.DotNet.Arcade.Sdk
 From Version 9.0.0-beta.24408.2 -> To Version 9.0.0-beta.24414.3
…813.2

Microsoft.SourceBuild.Intermediate.arcade , Microsoft.DotNet.Arcade.Sdk
 From Version 8.0.0-beta.24412.1 -> To Version 8.0.0-beta.24413.2
fixup semantic tokens tweask
…816.2

Microsoft.SourceBuild.Intermediate.arcade , Microsoft.DotNet.Arcade.Sdk
 From Version 9.0.0-beta.24414.3 -> To Version 9.0.0-beta.24416.2
…ence-packages build 20240815.3

Microsoft.SourceBuild.Intermediate.source-build-reference-packages
 From Version 9.0.0-alpha.1.24405.2 -> To Version 10.0.0-alpha.1.24415.3
davidwengier and others added 18 commits September 9, 2024 14:12
Most places already passed an ImmutableArray here. I left the method itself being an iterator, as 50% of callers need an ImmutableArray result, but the other 50% need an array, so no clear winner.
All callers did the conversion anyway
A clean error list is a good error list.
…ence-packages build 20240905.1

Microsoft.SourceBuild.Intermediate.source-build-reference-packages
 From Version 10.0.0-alpha.1.24428.1 -> To Version 10.0.0-alpha.1.24455.1
…tension method on RazorCodeDocument (dotnet#10851)

For a long while, the `GetLanguageKind(...)` method that determines
whether an index into a document falls within Razor, C# or HTML has been
a bit of a wart on the `IDocumentMappingService`. It really isn't part
of document mapping, and its implementation is completely distinct. In
fact, making the actual change is quite simple, so why hadn't it been
done yet? The answer is mocking.

There are several tests that mock
`IDocumentMappingService.GetLanguageKind(...)` to lie about test inputs.
In my not-so-humble opinion, this represents an abuse of mocking.
Instead of setting up tests to have the necessary inputs that ensure
`GetLanguageKind(...)` would return a real and correct result, the
inputs would often be garbage and an `IDocumentMappingService` mock
would lie about the `GetLanguageKind(...)` result at a particular
location. This makes moving `GetLanguageKind(...)` off of
`IDocumentMappingService` a much larger change than it needs to be. This
is why there are substantial test changes in this PR.

Don't misunderstand me as a mocking hater! Mocking libraries are
definitely useful! In fact, there are new mocks used in this very PR!
However, mocks should be used judiciously and thoughtfully, and in this
case, a mock was used to write lazy tests.

Fixes dotnet#8774
dotnet#10858)

This pull request updates the following dependencies

[marker]: <> (Begin:011df26a-fbd1-45b0-94b9-08db3601dcca)
## From https://github.com/dotnet/source-build-reference-packages
- **Subscription**: 011df26a-fbd1-45b0-94b9-08db3601dcca
- **Build**: 20240905.1
- **Date Produced**: September 5, 2024 10:13:43 PM UTC
- **Commit**: ad3c9aa85596f42c6a483233c50fab8cee8c412a
- **Branch**: refs/heads/main

[DependencyUpdate]: <> (Begin)

- **Updates**:
-
**Microsoft.SourceBuild.Intermediate.source-build-reference-packages**:
[from 10.0.0-alpha.1.24428.1 to 10.0.0-alpha.1.24455.1][1]

[1]:
dotnet/source-build-reference-packages@6bcf90f...ad3c9aa

[DependencyUpdate]: <> (End)


[marker]: <> (End:011df26a-fbd1-45b0-94b9-08db3601dcca)
…dit` (dotnet#10855)

Fixes dotnet#10842

The formatting self-nerd-sniping continues.

The formatting engine was written to use the LSP `TextEdit` class, which
makes some sense, but also uses Roslyn APIs like `SourceText` a lot,
which uses the `TextChange` struct instead. This meant lots of code to
convert to and from the two types. Changing the whole formatting engine
over to `TextChange`, and using more `TextSpan`, `LinePositionSpan` etc.
removes a lot of this code. It also makes a lot more sense in cohosting,
to boot.

I wouldn't claim that I've gone through and improved the perf of the
formatting engine, but rather I've use the changes to lead me to things
that need fixing. ie, I started out moving from `TextEdit[]` to
`ImmutableArray<TextChange>`, and this let me to places where pooled
array builders could be used, and places where `Range` and `Position`
were used which didn't make much sense, and then the constructor for
`LinePosition` threw at one point because it turns out we were only
using the `Line` property from the `Position` that used to be used, and
so never validated the characters, so that API moved to `int`, etc.

TL;DR the commits tell the story, and there could well be something I
missed, if it never came across my plate for another reason.
…enizer

* upstream/main: (270 commits)
  Fix after merge
  PR Feedback
  Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20240905.1
  Fix auto insert service after merge from main
  PR Feedback
  Use ImmutableArray in SourceTextDiffer too
  Create a helper method and revert change to shared code, just in case
  Convert HtmlFormatter to ImmutableArray<TextChange>
  IEnumarable to ImmutableArray
  Remove some more usage of LSP types, and simplify ranges to line numbers
  Use pooled collections in a few more spots
  Extract common code to helper method
  Rename some methods to Try... pattern
  Rename some variables etc.
  Fix broken tests
  Get all consuming code compiling again
  Convert TextEdit to TextChange
  Remove flaky test
  OnAutoInsert Cohosting Tests (dotnet#10829)
  Update GetLanguageKind(...) tests and move to Workspaces.Test
  ...
@333fred 333fred requested review from a team as code owners September 10, 2024 21:40
Comment on lines +66 to +68
public CSharpParseOptions CSharpParseOptions => _options.CSharpParseOptions;

public bool StartOfLine { get; set; } = true;
public bool ParseLeadingDirectives => _options.ParseLeadingDirectives;
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved these to pull from _options, rather than being auto props.


var context = new ParserContext(source, Options);
using var context = new ParserContext(source, Options);
Copy link
Member Author

Choose a reason for hiding this comment

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

Adjacent line changes required merging here.

@333fred
Copy link
Member Author

333fred commented Sep 10, 2024

@dotnet/razor-compiler for review

@jaredpar
Copy link
Member

Will call out the merge locations.

You could tokenize them.

@@ -203,9 +203,10 @@ public TestHtmlMarkupParser(ParserContext context)
EnsureCurrent();
}

public void Dispose()
void IDisposable.Dispose()
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because I implemented IDisposable on the base class, which conflicts with this derived implementation.

@333fred 333fred merged commit 87dafaf into dotnet:features/roslyn-tokenizer Sep 12, 2024
12 checks passed
@333fred 333fred deleted the features/roslyn-tokenizer branch September 12, 2024 20:58
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.