Skip to content

Fully immutable spancontexts#8245

Merged
333fred merged 4 commits intodotnet:mainfrom
333fred:immutable-contexts
Feb 18, 2023
Merged

Fully immutable spancontexts#8245
333fred merged 4 commits intodotnet:mainfrom
333fred:immutable-contexts

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 required bool AutoCompleteAtEndOfSpan { get; init; }

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
Member

@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