Skip to content

Conversation

CyrusNajmabadi
Copy link
Member

Fixes #78593

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner May 30, 2025 21:49
@@ -476,10 +476,7 @@ private void ScanSyntaxToken(ref TokenInfo info)
//
// Move back one space to see what's before this dot and adjust accordingly.

this.TextWindow.Reset(atDotPosition - 1);
var priorCharacterIsDot = this.TextWindow.PeekChar() is '.';
this.TextWindow.Reset(atDotPosition);
Copy link
Member Author

Choose a reason for hiding this comment

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

the core issue here is that we are shiftin gthe sliding text window around and the sliding text window keeps track of the location (as an offset) where the 'current token starts'. this data was not being appropriately moved around in teh case where we actually had to load new data into the sliding text window.

I thought about updating the window to support that. But it was complex and seemed easy to get wrong. It seemed easier to just stop actually shifting the window around. This was the only case where we do this while lexing a token. All the rest of the lexer instead moves the window around before/after lexing a token, so the token start is aways correct wrt to the window state.

Instead, i just added a helper to do exactly what we need here: get the previous character.

Copy link
Contributor

@AlekseyTs AlekseyTs Jun 2, 2025

Choose a reason for hiding this comment

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

this data was not being appropriately moved around in teh case where we actually had to load new data into the sliding text window.

This was the only case where we do this while lexing a token. All the rest of the lexer instead moves the window around before/after lexing a token, so the token start is aways correct wrt to the window state.

Is it possible to detect the "dangerous" sequence of operations and add an assert of some kind?

// _basis describes where in the source text the current chunk of characters starts at.
// So if the desired position is ahead of that, then we can just read the value out of
// the character window directly.
return this.CharacterWindow[_offset - 1];
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 common case. The "previous character" is still within the chunk of characters from the source-text that is in the sliding window.

Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

A few minor comments, nothing huge.

// Just go back to the source text to find this character. While more expensive, this should
// be rare given that most of the time we won't be calling this right after loading a new text
// chunk.
return this.Text[desiredPosition];
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: we could also simplify the entire method to just this call. However, it means that any time we see .num we will call back into the source-text. That was a bit concerning to me in the case where maybe there were tons of .1-like numbers in a file, and we were having to call all the way back through the source-text for each.

But if we have an easy way to validate performance against a large code-base, we could just try that and go with the much simpler route if we don't see a perf problem. it would certainly make this method trivial to verify :)

@CyrusNajmabadi
Copy link
Member Author

@dotnet/roslyn-compiler Corruption of a tree. Note: it needs a very special combination of factors to strike. Basically that we end up with the start of a .X floating point number at the 0th position in our 'sliding text window', causing a funky shifting-dance there to corrupt state.

I expect this to be very rare in practice. Though users with more .X FP numbers in a large file will have a higher chance of hitting this.

As this causes corruption, i imagine it may be in consideration for servicing. However, at the same time, we've only gotten one customer report in the 8 months since the original PR went in. So perhaps it is too low priority. @jaredpar for thoughts.

@jaredpar
Copy link
Member

@AlekseyTs, @jcouv PTAL

{
Debug.Assert(this.Position > 0);
var desiredPosition = this.Position - 1;
if (desiredPosition >= _basis && _offset > 1)
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 2, 2025

Choose a reason for hiding this comment

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

_offset > 1

Can this ever be false? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

yes. it's def possible for _offset to be zero. In the case we're testing, that's what is actually happening.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just want to make sure there was no confusion about the question. We calculated desiredPosition = this.Position - 1 and ensured that desiredPosition >= _basis. Can _offset >= 1 still be false?

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jun 2, 2025

Done with review pass (commit 6) #Closed

{
Debug.Assert(this.Position > 0);
var desiredPosition = this.Position - 1;
if (desiredPosition >= _basis && _offset > 1)
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 2, 2025

Choose a reason for hiding this comment

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

desiredPosition >= _basis && _offset > 1

Would checking just _offset > 1 be sufficient? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think that would be sufficient. Here's my reasonining:

The _offset is simply telling us where the read should happen in the sliding text window. While the basis tells us where in the _text the current chunk was read from. So imagine the following:

We read a chunk from text at position 500 in the text, and the sliding text window has length 1000, and we read that chunk in from characters 250-750. And, currently, the offset is simply at 250.

in this case, the _basis will be 500. The desired position will be 499. And the _offset will be 250. If we just check _offset>1 we will think we can read charWindow[offset-1]. But that will read at position 249 which will be a random value from a prior filling of hte buffer.

In other words, we only want to read one char back if the position we're reading from is know for certain to be a character that was in teh current chunk read from the _sourceText (which _basis) tells us.

LMK if that makes sense, or if you think i'm off somewhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

in this case, the _basis will be 500. The desired position will be 499. And the _offset will be 250. If we just check _offset>1 we will think we can read charWindow[offset-1]. But that will read at position 249 which will be a random value from a prior filling of hte buffer.

I think I am missing something. Given the quote, I am failing to see the relationship between the position and the _offset. Why would the _offset be 250 when position is 499? If position is 505, can _offset still be 250? And, if so, why do we think that position 505 and _offset 249 point at the same character?

Copy link
Member Author

Choose a reason for hiding this comment

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

position just tells us where in the original text the current chunk corresponds to. _offset is the start of that chunk within the character window. THey're referencing different regions in different storage locations.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then why does it ever make sense to read from _offset - 1?

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 position is 499? If position is 505, can _offset still be 250?

Yes. Position=499 means "i want to read the original character at position 499 in the Sourcetext". _offset=250 is saying "a read of the current position will be read from the text window array at index 250 in teh array".

Copy link
Member Author

Choose a reason for hiding this comment

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

Then why does it ever make sense to read from _offset - 1?

_offset is where the current char at position this.Position is within the char-window. _offset + 1 is where the next char is (and so on), _offset - 1 is where the prior char is. (and so and so forth for 2, 3, 4, ... or -2, -3, -4, etc). This is fine as long as that _offset +/- N is still within the portion of the char-window that maps to the current chunk.

So, in the normal/common case. It is fine to read the char that immediately precedes _offset. That's normally what the last lex did anyways :). But in this special case, we lexed the last token, then shifted everything around so that [`.`, `0`, ... is the start of the window. And in that rare case, there's no "prior char" to read in teh window. So we fall back to going back into the SourceText itself to read it.

Copy link
Contributor

Choose a reason for hiding this comment

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

_offset is where the current char at position this.Position is within the char-window.

Then it would seem that this.Position - 1 >= _basis and _offset - 1 >= 0 should always be equal to each other. Is this not correct? What am I missing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this not correct?

I'm not certain if it is correct or if it not. I don't see any documentation guaranteeing this. I'm wary about drawing unstated conclusions. (That was one of the problems that led us here for example. For example that it was unstated that you can't reset while in the middle of lexing a token).

I can simplify to that code if you want. If you're confident it will always be correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not certain if it is correct or if it not. I don't see any documentation guaranteeing this. I'm wary about drawing unstated conclusions. (That was one of the problems that led us here for example. For example that it was unstated that you can't reset while in the middle of lexing a token).

I can simplify to that code if you want. If you're confident it will always be correct.

When I inline and simplify parts of this expression I get the following:

desiredPosition >= _basis && _offset >= 1
(this.Position - 1)>= _basis && _offset >= 1
((_basis + _offset) - 1)>= _basis && _offset >= 1
((_offset) - 1)>= 0 && _offset >= 1
_offset >= 1 && _offset >= 1
_offset >= 1

It feels like we should be using the simplest form of the condition. Do you agree with the reasoning or still have doubts?

@CyrusNajmabadi
Copy link
Member Author

Consider using _offset > 0 then.

sure.

private readonly SlidingTextWindow _window = window;

public void SetDefaultCharacterWindow()
=> _window._characterWindow = new char[DefaultWindowLength];
Copy link
Member Author

Choose a reason for hiding this comment

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

needed to do this as the backing char window for the lexer comes from a pool, and thus can see different length'ed arrays depending on if a larger array was returned to the pool at some point. this allows the test to keep things isolated from other tests changing shared static state.

@@ -480,5 +495,16 @@ internal static char GetCharsFromUtf32(uint codepoint, out char lowSurrogate)
return (char)((codepoint - 0x00010000) / 0x0400 + 0xD800);
}
}

public TestAccessor GetTestAccessor()
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 5, 2025

Choose a reason for hiding this comment

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

public

internal? #Closed

private readonly SlidingTextWindow _window = window;

public void SetDefaultCharacterWindow()
=> _window._characterWindow = new char[DefaultWindowLength];
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 5, 2025

Choose a reason for hiding this comment

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

_characterWindow

Do we need to worry about _characterWindowCount value? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

Not in this case. _characterWindowCount is the count of usable characters in the array. It starts at 0 (as no data has actually been read into it). In this case, we want it to stay zero.

Note: i have an alternate branch where i switch to using an ArraySegment to better represent what's going on in this type. But it seemed larger in impact, so i scrapped it for this narrow fix. There's a lot we could do with modern C#/.Net to greatly simplify this type. but i felt that the gain wasn't there for something we touch so rarely, and that the review cost would be too high.

If you'd like more cleanup here (it ends up cutting down the amount of code by several hundred lines) let met know and i can consider resurrecting.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we are good as is.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changes in the simplified version:

  1. moved to a simple array-segment, wrapping an array that is large, but smaller then LOH size. No real point in limiting ourselves to this 2k buffer.
  2. Lexeme info is stored in the lexer not the sliding text window. The window is just there for easy, fast, access to contiguous memory.
  3. The common case is that every token is realized outside from teh contents of the window. The extremely rare case (around 1:10,000 tokens) does a token potentially cross out of the buffer. And, in that case, we fetch from the underlying _text.
  4. Pointers/references are readonly, so no need to move them around in the case of resizing/access

I also played around with moving to a purely stack-based system for lexing. But this ended up being more involved as we unfortunately use inheritance to model different lexing behaviors. Ideally, lexing could operate almost entirely on stack-based memory, using spans. But that was even more involved. Given our priorities and how rare lexer issues are, i opted against going this route. Even though the wins were there, the amount of churn made me feel like it was highly possible a bug might creep in.

Copy link
Member Author

Choose a reason for hiding this comment

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

wfm.

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jun 5, 2025

Done with review pass (commit 25) #Closed

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 28)

@AlekseyTs
Copy link
Contributor

Please squash commits while merging.

@CyrusNajmabadi CyrusNajmabadi enabled auto-merge (squash) June 5, 2025 13:52
@CyrusNajmabadi CyrusNajmabadi merged commit 70435cc into dotnet:main Jun 5, 2025
23 of 24 checks passed
@CyrusNajmabadi CyrusNajmabadi deleted the parsingBug branch June 5, 2025 15:10
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Jun 5, 2025
333fred added a commit that referenced this pull request Jun 17, 2025
* Simpli

* Simplify

* Simplify

* docs

* docs

* Clear

* Update src/Workspaces/Core/Portable/FindSymbols/Shared/AbstractSyntaxIndex_Persistence.cs

* Remove method

* Fix ILBuilder visualization (#78764)

Turns out that we do have visualization of in-progress IL building, but it's been broken since 2020, when we renamed Roslyn.Test.Utilities to Microsoft.CodeAnalysis.Test.Utilities. It was then further broken by the change a few months ago to pass IL visualization formats along. This gets the debugger display working again.

* Remove EditorFeaturesWpf TestComposition (#78769)

After the EditorFeatures.WPF => EditorFeatures refactoring, this TestComposition became equivalent to the EditorFeatures one, so just removing the WPF one.

* Fix renames across different extensions

* Add docs

* Add docs

* Update src/VisualStudio/Core/Def/PackageRegistration.pkgdef

* Explicitly reset

* Reset state

* Add docs

* Simplify

* Revert

* Simplify

* Simplify

* Docs

* Update src/VisualStudio/Core/Def/Commands.vsct

Co-authored-by: David Barbet <[email protected]>

* Update src/VisualStudio/Core/Impl/SolutionExplorer/SymbolTree/SymbolItemContextMenuController.cs

Co-authored-by: David Barbet <[email protected]>

* Simplify

* Simplify

* multi notify

* only if it changed

* Don't move backwards

* Docs

* Update src/VisualStudio/Core/Impl/SolutionExplorer/SymbolTree/SymbolTreeChildCollection.cs

* Ensure sln load uses project absolute paths

* lint

* Rename field to make it clearer what it's used for

It's only for dynamic files, so make that clear.

* Don't refresh dynamic files under a lock

When we were getting the notification of a dynamic file being changed,
we were taking the project and workspace locks before fetching the
actual content. This is really expensive now that the fetching itself
might not be instant.

Fundamentally this change is just switching the work to acquire the
locks to get the setup info, then releasing the locks and calling the
provider to get the info. I'm putting this around a batching queue so
we can deduplicate lots of events at once, and also to ensure ordering
so if we were to try refreshing the document more than once we don't
end up in a case where we have two threads trying to put different
versions of the document into the workspace and getting the ordering
wrong.

Fixes #78734

* Fix cast before we unsubscribe

This was otherwise throwing an exception every shutdown.

* Adjust implementation to respect diagnostic flag and update tests

* Remove workarounds for bugs that have been fixed

Closes #77995

* Remove a workaround we had for an old version of Copilot

This bug was fixed a long time ago.

* Switch return to continue

* Fixed multi-variable declaration support in RemoveUnnecessarySuppressions.

* Removed whitespace.

Co-authored-by: DoctorKrolic <[email protected]>

* Simplified TestRemoveDiagnosticSuppression_Attribute_MultiVariableDeclaration with inline data.

* Expanded remove unnecessary suppression tests, removed unnecessary loop and fixed partial method/property support in AbstractRemoveUnnecessaryInlineSuppressionsDiagnosticAnalyzer.

* Directly create virtual project when dotnet run-api is missing for now

* Avoid dereferencing null CheckConstraintsArgs.CurrentCompilation (#78729)

Fixes #78430.

* Updates to unblock dartlab pipeline (#78793)

* update timeout

* wip

* Add main to main-vs-deps flow (#78798)

* Create branch-merge2.jsonc

* Update main-merge.yml

* Rename branch-merge2.jsonc to branch-merge-2.jsonc

* Update PublishData.json

* Update and rename branch-merge.jsonc to main-to-main-vs-deps-branch-merge.jsonc

* feedbacl

* make clear main is for validation

* EnC: Simplify diagnostic reporting (#78708)

* Refactoring of extension methods in source packages (#78620)

* Change namespace of OneOrMany and TemporaryArray to MS.CA.Collections

* Cleanup Enumerable and ImmutableArray extensions

* Update imports/usings

* Fix VB remove-imports not showing up

* add the schedule back to main-merge (#78807)

* Fix LSP references for using alias

* Clean up HasDuplicates extension method (#78808)

* Extensions: Do not consider extension block methods as entry points, consider their implementation methods instead (#78667)

* Decouple EditorFeatures.Test.Utilities from VS services

Microsoft.CodeAnalysis.EditorFeatures.Test.Utilities depended on
Microsoft.VisualStudio.Editor which brought along various VS layer
services. Further, we had some stub implementations of those VS services
there. Digging into this this coupling wasn't really necessary:

1. There were EditorFeatures tests using the editor adapters stub,
   but those tests pass just fine without it, so that was easy to
   move to the VS specific tests.
2. StubVsServiceExporters didn't appear to be used either, so unsure
   why those were in this layer at all. Those were also moved.
3. We had implementations of some settings services, which it's easier
   to just delete and remove the options persister from the composition
   in the first place. This also better ensures that we don't have
   code that might try creating registry keys from ILocalRegistry
   which we absolutely don't want.

* Extensions: mark skeleton type with 'specialname' (#78690)

* Fix await completion in an async iterator

* Extensions: fix lowering of implicit conversion on receiver in pattern-based scenarios (#78685)

* Add test

* Find metadata reference for alias

* Add a fallback path when launching the BuildHost

* refactor

* Added notes to update GetBuildHostPath if packaging changes

* Update comments

* Add heuristic for the loaded from nuget package scenario

* Fix corruption of sliding text window when trying to peek backwards.  (#78774)

Co-authored-by: Fred Silberberg <[email protected]>

* Track changed text instead of clearing immediate window.

* EnC: Partial solution updates (#78744)

* Restore some parts of the progression api some legacy components (like codemap) use.

* Remove unused internal APIs

* Remove unused internal APIs

* Remove unused internal APIs

* Remove unused internal APIs

* Make static

* Simplify

* Avoid ignored directive errors in disabled regions (#78158)

* Extensions: use specific tracking issues for different areas (#78834)

* Extensions: relax inferrability rule (#78758)

* Remove blank line output from the node writer proportional to the number of non-abstract nodes (#78844)

* Revert "Refactoring of extension methods in source packages (#78620)" (#78850)

This reverts commit 917401d, broke typescript

* Switch off of mutlidictionary (#78851)

This is from ICSharpCompiler, and is therefore causing unnecessary dll loads.

* Switch behavior of "Go to definition" and "Go to implementation" for partial members

* Fix LSP tests

* Fix peek test

* Switch to existing helpers for multi-dictionary use

* call TryGetValue to get dictionary entry (#78863)

* Remove more progression code

* Remove more progression code

* Remove more progression code

* Remove more progression code

* Remove more progression code

* Remove more progression code

* Remove more progression code

* Remove more progression code

* Add VSTypeScriptAsynchronousTaggerProvider2 for TypeScript that avoids TemporaryArray (#78866)

* Remove more progression code

* Remove more progression code

* Remove more progression code

* Remove more progression code

* Fix build break (#78870)

* Add more ETW events to trace assembly loading (#78840)

* Add more ETW events to trace assembly loading

* Update src/Compilers/Core/Portable/CodeAnalysisEventSource.Common.cs

Co-authored-by: Joey Robichaud <[email protected]>

* Put correct alc reference

* Use binary literals

---------

Co-authored-by: Joey Robichaud <[email protected]>

* Introduce EA layer for TS to integrate with classification

* Simplify

* Add CommandLineResource API (#78679)

* Revert "Revert "Refactoring of extension methods in source packages (#78620)" (#78850)"

This reverts commit e083be9.

* Fix

* Revert "Fix"

This reverts commit 574935d.

* Revert "Revert "Revert "Refactoring of extension methods in source packages (#78620)" (#78850)""

This reverts commit 27ae586.

* Lint

* Remove all progression code

* Revert

* remove code

* Fix

* Reduce allocations under ContextMutableIntervalTree (#78885)

There are some closure allocations unnecessarilly happening during ContextMutableIntervalTree construction. Simply changed a couple methods to static and added a parameter for the extra bit of data they need to get rid of those allocations.

This is a very small improvement, only about 0.1% (6 MB) of allocations during typing in the razor speedometer test.

* Reduce allocations in the ImageElementConverter and ImageIdConverter Read methods (#78881)

* Reduce allocations in the ImageElementConverter and ImageIdConverter Read methods

These methods show up in the typing scenario in the razor speedometer test as about 0.9% (63 MB) of allocations.

1) Changed ImageIdConverter to be more like ImageElementConverter and not create a JsonDocument object to query
2) Changed several Utf8JsonReader.GetText calls to instead use Utf8JsonReader.CopyString
3) Changed JsonElement.GetString and new Guid(...) to instead use Utf8JsonReader.GetGuid()

Note that if this PR is merged, I'll also try to make a change to vslanguageserverclient to also do the same as that code has the same issues.

* Fix issue serializing null options in VB

* Yield in task continuation to prevent stack overflow

* Move to .NET 10 Preview 5

* Remove RemoteControl workaround

* Lint response

* Lint response

* Lint response

* Lint response

* Lint response

* Fix issue with not resetting valueLength (#78890)

* Fix issue with not resetting valueLength

#78881 was just merged with a small bug, in that valueLength wasn't reset before it was used a second time. If the typeName is > 64 chars, then this would have thrown a different exception than it should have down a couple lines.

* remove unused method

* Lint response

* Lint response

* fix compiler side

* Simplify workspace hookup in syntactic tagger

* Move

* Switch to ITextBuffer2

* REvert

* REvert

* Update src/EditorFeatures/Core/InlineDiagnostics/AbstractDiagnosticsTaggerProvider.cs

* Update src/EditorFeatures/Core/StringIndentation/StringIndentationTaggerProvider.cs

* Update src/EditorFeatures/Core/Tagging/AsynchronousViewportTaggerProvider.cs

* REvert

* remove more workspace registration code

* Move to .NET 10 Preview 5 (#78906)

* Move to .NET 10 Preview 5

* Lint response

* Lint response

* Lint response

* Lint response

* Lint response

* remove unused method

* Lint response

* Lint response

* fix compiler side

* more

---------

Co-authored-by: Cyrus Najmabadi <[email protected]>

* Fix

* Inline Hints - do not allow double click for collection expression type hint (#78900)

* do not allow double click for collection expression hint

* Update src/EditorFeatures/Test2/InlineHints/CSharpInlineTypeHintsTests.vb

Co-authored-by: Cyrus Najmabadi <[email protected]>

* comment

---------

Co-authored-by: Cyrus Najmabadi <[email protected]>

* Add tests

* Revert "Update code to use null propagation (#78733)"

This reverts commit d785549, reversing
changes made to a8808be.

* Revert "Move to .NET 10 Preview 5 (#78906)"

This reverts commit a7fa681.

* fix warning

* Revert "Update to using unbound nameof(X<>) expressions (#78731)"

This reverts commit 6a09280, reversing
changes made to a7fa681.

* Revert "Update to using simple untyped lambdas (#78732)"

This reverts commit a8808be, reversing
changes made to 6a09280.

* Fix OOP crash issue with copilot change analysis

* Re-enable IDE0051 (#78919)

This re-enables the unused member analyzer and removes all of the
methods it flagged as unused.

* Extensions: address some follow-up comments (#78847)

* Pass missing Hot Reload result properties (#78929)

* Fix deadlock if an MSBuild task is writing to stdout

When we switched over to communicating over a named pipe rather than
stdin/stdout, we were still redirecting stdin and stdout. This had
the side effect that if a build task was directly writing to standard
out, the build would eventually deadlock since we weren't reading from
the other side.

I thought about simply not redirecting stdin/stdout, but I could imagine
other problems might come up if we were to have multiple build hosts
trying to share stdin/stdout. So now we'll log stdout the same way
we log stderr, and explicitly close stdin so readers won't deadlock
waiting for input.

Fixes #78766

* Revert

* Collect stats

* Extensions: allow cref references to extension members (#78735)

* Fix scoped variance checks involving ref struct interface implementation (#78883)

* Remove now unused Progression references and related hacks

* Expose a couple of things to Razor

* Remove more unneeded references

* Obsolete api

* [main] Source code updates from dotnet/dotnet (#78805)

* [VMR] Codeflow a528f84-a528f84

[[ commit created by automation ]]

* Update dependencies from https://github.com/dotnet/dotnet build 270315
No dependency updates to commit

* [VMR] Codeflow f5faea9-f5faea9

[[ commit created by automation ]]

* Update dependencies from https://github.com/dotnet/dotnet build 270450
No dependency updates to commit

* Update dependencies from https://github.com/dotnet/dotnet build 270603
No dependency updates to commit

* Update dependencies from https://github.com/dotnet/dotnet build 270662
No dependency updates to commit

* [VMR] Codeflow 86826d3-86826d3

[[ commit created by automation ]]

* Update dependencies from https://github.com/dotnet/dotnet build 271018
No dependency updates to commit

* [VMR] Codeflow 32a2620-32a2620

[[ commit created by automation ]]

* Update dependencies from https://github.com/dotnet/dotnet build 271181
No dependency updates to commit

* [VMR] Codeflow 25357a9-25357a9

[[ commit created by automation ]]

* Update dependencies from https://github.com/dotnet/dotnet build 271343
No dependency updates to commit

* Update dependencies from https://github.com/dotnet/dotnet build 271417
No dependency updates to commit

* Revert incorrect Roslyn.sln changes

---------

Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Viktor Hofer <[email protected]>

* Code simplification

* Specify single reducer

* Simplify

* Add test

* Only add annotations to potential references

* :Extract type

* Docs

* move up

* Expose `IsIterator` as a public API (#78813)

* Expose `IsIterator` as a public API

* Fix workspace generated method symbol implementation

* Implement new property in yet another non-compiler-owned method symbol implementation

* SemanticSearch.ReferenceAssemblies

* Verify public code path in C#

* Add negative tests

* Add C# local function tests

* Test VB lambda iterators

* Revert accessibility change

* Disable warnings

* Use explicit type

* Simplify lambda tests

* Test interface property

* Add additional VB case

* Update enum values

* PR Feedback

---------

Co-authored-by: Cyrus Najmabadi <[email protected]>
Co-authored-by: Cyrus Najmabadi <[email protected]>
Co-authored-by: Todd Grunke <[email protected]>
Co-authored-by: David Barbet <[email protected]>
Co-authored-by: David Barbet <[email protected]>
Co-authored-by: Jason Malinowski <[email protected]>
Co-authored-by: John Douglas Leitch <[email protected]>
Co-authored-by: DoctorKrolic <[email protected]>
Co-authored-by: Rikki Gibson <[email protected]>
Co-authored-by: AlekseyTs <[email protected]>
Co-authored-by: Ankita Khera <[email protected]>
Co-authored-by: Tomáš Matoušek <[email protected]>
Co-authored-by: Julien Couvreur <[email protected]>
Co-authored-by: Joey Robichaud <[email protected]>
Co-authored-by: Jan Jones <[email protected]>
Co-authored-by: DoctorKrolic <[email protected]>
Co-authored-by: Chris Sienkiewicz <[email protected]>
Co-authored-by: Joey Robichaud <[email protected]>
Co-authored-by: tmat <[email protected]>
Co-authored-by: Jared Parsons <[email protected]>
Co-authored-by: David Wengier <[email protected]>
Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Viktor Hofer <[email protected]>
@jaredpar
Copy link
Member

jaredpar commented Jul 7, 2025

/backport to release/dev17.14

Copy link
Contributor

github-actions bot commented Jul 7, 2025

Started backporting to release/dev17.14: https://github.com/dotnet/roslyn/actions/runs/16124141333

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unexpected output from Microsoft.CodeAnalysis.CSharp.SyntaxTree.ToString() after calling GetCompilationUnitRoot()
5 participants