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

Fully immutable spancontexts #8245

Merged
merged 4 commits into from
Feb 18, 2023
Merged

Conversation

333fred
Copy link
Member

@333fred 333fred commented Feb 9, 2023

Makes all span contexts fully immutable, and converts to a builder pattern for creating them during parsing.

@333fred 333fred force-pushed the immutable-contexts branch 3 times, most recently from 47c1cbc to 665ca58 Compare February 9, 2023 18:41

public Func<string, IEnumerable<Syntax.InternalSyntax.SyntaxToken>> Tokenizer { get; set; }

public static SpanEditHandler CreateDefault()
Copy link
Member Author

Choose a reason for hiding this comment

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

When refactoring, I found the implicit AcceptedCharactersInternal values confusing, so I removed this overload.


public string AutoCompleteString { get; set; }
public string AutoCompleteString => _autoCompleteStringAccessor.CanAcceptCloseBrace ? "}" : null;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the only place where we were using the mutation capabilities of contexts, and it was really just delayed initialization. I've replaced it with an accessor that verifies only one set occurs, and verifies that the set occurred before access.

@333fred 333fred force-pushed the immutable-contexts branch from 7555723 to 7ba0da4 Compare February 9, 2023 23:34
@333fred 333fred marked this pull request as ready for review February 10, 2023 17:55
@333fred 333fred requested review from a team as code owners February 10, 2023 17:55
@333fred
Copy link
Member Author

333fred commented Feb 10, 2023

@dotnet/razor-compiler for review please.


public string AssemblyName { get; set; }
public string AssemblyName { get; }

public List<RazorDiagnostic> Diagnostics { get; }
Copy link
Member

Choose a reason for hiding this comment

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

Is the expectation that this becomes ImmutableArray<ReportDiagnostic> in the future?

Copy link
Member Author

Choose a reason for hiding this comment

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

My expectation is that this gets removed entirely at some point 🙂.

Copy link
Member

@DustinCampbell DustinCampbell left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@chsienki chsienki left a comment

Choose a reason for hiding this comment

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

LGTM

Only one context needs a bit of delayed initialization, the AutoComplete context, as it needs to know whether or not to accept a closing brace during partial reparsing. Aside from this single instance, all other contexts were used completely immutably, and now reflect this.
@333fred 333fred enabled auto-merge (squash) February 18, 2023 00:15
@333fred 333fred merged commit 7e79f9d into dotnet:main Feb 18, 2023
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.

5 participants