Use elastic crlf where applicable by Therzok · Pull Request #25929 · dotnet/roslyn

Therzok · 2018-04-04T18:31:07Z

The changes switch End of Line trivia to use Elastic on code paths which end up being formatted by code actions or explicit elastic annotation formatting

Details

### Customer scenario

See repro in #25927

Bugs this fixes

#25927

Workarounds, if any

Reformat the piece of code

Risk

?

Performance impact

Only modified the crlf trivia in case of code going into code actions. That triggers elastic trivia formatting, even if the code itself is not marked to format via annotation.

Is this a regression from a previous update?

Root cause analysis

I'm not sure how to add tests for this.

How was the bug found?

Found via ad-hoc testing in vsmac.

Neme12 · 2018-04-04T18:35:53Z

Have you also checked references of SyntaxGenerator.CarriageReturnLineFeed?

Therzok · 2018-04-04T18:45:09Z

Yes, it is not used.

cc @sharwell @CyrusNajmabadi

CyrusNajmabadi · 2018-04-04T18:55:22Z

This needs @dotnet/roslyn-compiler review as we're touching the compiler layer. Also @mattwar for feedback. This will need tests of appropriate systems.

Therzok · 2018-04-04T18:57:07Z

The first commit in the PR is the one that fixes the github bug I reported.

The others are code actions which are not marked with Formatter.Annotation so I judged that adding elastic eol would get them fixed as a catch all in:
http://source.roslyn.io/#Microsoft.CodeAnalysis.Workspaces/CodeActions/CodeAction.cs,275

Therzok · 2018-04-04T19:02:42Z

I have removed the elastic crlf change in SyntaxNodeRemover. It was an oversight when adding changes to the commit, elastic markers are added post-fact.

jcouv · 2018-04-04T20:34:01Z

There no longer is any compiler change in this PR, so I removed Compiler tag. Thanks

Therzok · 2018-04-04T20:38:50Z

Weird failure here:
https://ci.dot.net/job/dotnet_roslyn/job/dev15.7.x/job/windows_release_unit64_prtest/1050/testReport/junit/Microsoft.CodeAnalysis.Editor.UnitTests.SolutionCrawler/WorkCoordinatorTests/FileFromSameProjectTogetherTest/

Added another commit to fix unit tests. It seems like they add another newline now.

Therzok · 2018-04-04T20:43:54Z

...VisualBasic/Portable/IntroduceVariable/VisualBasicIntroduceVariableService_IntroduceLocal.vb

This is different behaviour from C#. declarationStatement does not have this check.

As in, this probably causes the eol changes.

FWIW, the EOL changes are welcome IMO. The whole point of elastic trivia formatting was precisely for these cases.

jasonmalinowski · 2018-05-15T22:47:14Z

@Therzok @DustinCampbell To confirm, we're not taking this in 15.7? If so, we'll need to change the PR target.

Therzok · 2018-05-16T00:17:31Z

Sure. I'll retarget

Therzok · 2018-05-21T20:48:16Z

Which branch should I target this PR to?

Fixes dotnet#25927

Document is formatted as part of the code fix

DustinCampbell

The change looks good to me. However, the integration tests need to be looked at. I expect many of them are failing because fixes/refactorings are now inserting line breaks where they didn't before.

DustinCampbell

The change looks good to me. However, the integration tests need to be looked at. I expect many of them are failing because fixes/refactorings are now inserting line breaks where they didn't before.

Therzok · 2018-05-22T18:05:37Z

Yeah, I rebased in order to get a new build. I had fixed the previous ones before having to retarget. I'll fix asap

Therzok · 2018-05-22T20:05:50Z

Aren't the test failures related to #27048 ?

etbyrd · 2018-05-22T21:23:28Z

Yes, I can confirm that the tests are failing because of the ongoing issue in #27048

etbyrd · 2018-05-24T22:48:40Z

Ping - the new tests have completed.

However, I have identified that

Roslyn.VisualStudio.IntegrationTests.CSharp.CSharpReplIdeFeatures.AddUsing
Roslyn.VisualStudio.IntegrationTests.CSharp.CSharpReplIdeFeatures.QualifyName

Are failing across multiple PR's are probably flaky/broken

Therzok · 2018-05-30T03:55:24Z

@DustinCampbell anything else needed?

DustinCampbell · 2018-06-05T14:51:34Z

...VisualBasic/Portable/IntroduceVariable/VisualBasicIntroduceVariableService_IntroduceLocal.vb

FWIW, the EOL changes are welcome IMO. The whole point of elastic trivia formatting was precisely for these cases.

jinujoseph · 2018-06-05T15:54:41Z

Approved to merge for 15.8.Preview3

Therzok requested review from a team as code owners April 4, 2018 18:31

Therzok mentioned this pull request Apr 4, 2018

Mixed new lines after adding using statement mono/monodevelop#4409

Open

Therzok force-pushed the elastic-crlf branch from c3d1d95 to 83ca47d Compare April 4, 2018 19:02

Therzok force-pushed the elastic-crlf branch from 83ca47d to 5a49193 Compare April 4, 2018 19:04

jcouv added Area-IDE Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. and removed Area-Compilers labels Apr 4, 2018

Therzok commented Apr 4, 2018

View reviewed changes

jinujoseph added this to the 15.8 milestone Apr 15, 2018

jinujoseph assigned DustinCampbell Apr 30, 2018

Therzok changed the base branch from dev15.7.x to master May 21, 2018 20:59

Therzok force-pushed the elastic-crlf branch from b2b2d76 to d30bbf0 Compare May 21, 2018 20:59

Therzok requested a review from a team as a code owner May 21, 2018 20:59

Therzok force-pushed the elastic-crlf branch from d30bbf0 to 2e596ba Compare May 21, 2018 21:00

Therzok added 4 commits May 22, 2018 00:07

Fix newline that is being prepended to the first using via code fix

44d14a9

Fixes dotnet#25927

Use elastic crlf in c# convert to full prop

5ae9beb

Document is formatted as part of the code fix

Use elastic crlf in implement service, as it's formatted as code action

8389f14

More elastic crlf

5765721

Unit test changes

ddd9562

Therzok force-pushed the elastic-crlf branch from 2e596ba to ddd9562 Compare May 21, 2018 21:07

DustinCampbell approved these changes May 22, 2018

View reviewed changes

DustinCampbell requested changes May 22, 2018

View reviewed changes

etbyrd removed the Community The pull request was submitted by a contributor who is not a Microsoft employee. label May 26, 2018

DustinCampbell approved these changes Jun 5, 2018

View reviewed changes

jinujoseph added the Approved to merge label Jun 5, 2018

jinujoseph merged commit 5bef566 into dotnet:master Jun 5, 2018

Therzok deleted the elastic-crlf branch June 5, 2018 15:55

Conversation

Therzok commented Apr 4, 2018

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Uh oh!

Neme12 commented Apr 4, 2018

Uh oh!

Therzok commented Apr 4, 2018

Uh oh!

CyrusNajmabadi commented Apr 4, 2018

Uh oh!

Therzok commented Apr 4, 2018

Uh oh!

Therzok commented Apr 4, 2018

Uh oh!

jcouv commented Apr 4, 2018

Uh oh!

Therzok commented Apr 4, 2018

Uh oh!

Therzok Apr 4, 2018

Choose a reason for hiding this comment

Uh oh!

Therzok Apr 4, 2018

Choose a reason for hiding this comment

Uh oh!

DustinCampbell Jun 5, 2018

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski commented May 15, 2018

Uh oh!

Therzok commented May 16, 2018

Uh oh!

Therzok commented May 21, 2018

Uh oh!

DustinCampbell left a comment

Choose a reason for hiding this comment

Uh oh!

DustinCampbell left a comment

Choose a reason for hiding this comment

Uh oh!

Therzok commented May 22, 2018

Uh oh!

Therzok commented May 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

etbyrd commented May 22, 2018

Uh oh!

etbyrd commented May 24, 2018

Uh oh!

Therzok commented May 30, 2018

Uh oh!

DustinCampbell Jun 5, 2018

Choose a reason for hiding this comment

Uh oh!

jinujoseph commented Jun 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Therzok commented May 22, 2018 •

edited

Loading