Tagging @dotnet/roslyn-ide @dpoeschl

Tagging @jasonmalinowski as well.

@jasonmalinowski @dotnet/roslyn-infrastructure @jaredpar Do you know what is causing:

Publishing build artifacts failed with an error: Not found PathtoPublish: F:\vsagent\1\s\artifacts\bin\Microsoft.VisualStudio.LanguageServices.IntegrationTests\Release\net472\xUnitResults

?

@CyrusNajmabadi Once the test(s) are updated as described above, I have no objection to this change. However, I need to review the original linked bug http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/912965 to see if it contains any information that would block this behavior change.

@CyrusNajmabadi I reviewed the bug and it seems like the following tests need to be present. Some of them may already exist in the code but we should search for them to ensure coverage. If you believe an existing test covers one or more scenarios from below, please point out the test and mention the scenarios covered.

Typing tests

Sample code:

class TestClass
{
  void TestMethod(IDisposable obj)
  {
    using (obj)
    using (obj)
    using (obj)
    using (obj)
    {
    }
  }
}

Test cases:

1. Typing the code top-to-bottom without manual indentation changes should result in vertical alignment of all using keywords
2. Typing the code top-to-bottom without manual indentation changes should result in vertical alignment of { and } with the using keywords
3. Pressing Enter after any ) of a using statement in the sample should place the caret one indent more than the using keyword
4. Typing { after any variant of step 3 should vertically align the brace with the using keyword

Format document test

Input:

class TestClass
{
  void TestMethod(IDisposable obj)
  {
    using (obj)
      using (obj)
      using (obj)
      using (obj)
    {
    }
  }
}

Expected result:

class TestClass
{
  void TestMethod(IDisposable obj)
  {
    using (obj)
    using (obj)
    using (obj)
    using (obj)
    {
    }
  }
}

@CyrusNajmabadi I reviewed the bug and it seems like the following tests need to be present. Some of them may already exist in the code but we should search for them to ensure coverage.

Sure. I will add those today.

@sharwell tests added that validate behavior for both typing and for format-doc

@CyrusNajmabadi LGTM but I'm going to wait until Monday to ensure people have a chance to review the behavior change.

@sharwell could you get the relevant eyes on this?

@CyrusNajmabadi it was on schedule for last Monday but we ran out of time so it's moved to 18 March.

closing

Design review conclusion: overall, this seems like a reasonable change. I will file follow-up issues for two specific things:

1. Caused by this change: users who have automatic brace completion enabled and use Tab to complete the closing parentheses could end up in a state where the indentation of the using (true) line is incorrect. We don't believe this will cause problems, but if it does we could end up needing to address it in the future.
2. Not caused by this change, but observed when looking through the test sequences: we may have a test gap covering the formatting at the end of a line, when multiple spaces appear between ) and {, and the user presses enter somewhere in the middle of this. We should verify tests are present for these cases both for using statements and other types of statements that allow braces.

@CyrusNajmabadi there were some compilation errors here. I'm happy to correct them if you want, or you can update it yourself. Please let me know your preference. 😄

Feel free to take it on. Thanks!

@sharwell @jasonmalinowski not sure what's up with the determinism build. I'm not sure how i could have broken that. Error seems to be:

F:\workspace\_work\1\s\.tools\msbuild\16.0.0-alpha\tools\MSBuild\Current\Bin\Microsoft.Common.CurrentVersion.targets(2114,5): error MSB3277: Found conflicts between different versions of "Newtonsoft.Json" that could not be resolved.  These reference conflicts are listed in the build log when log verbosity is set to detailed. [F:\workspace\_work\1\s\src\Tools\ExternalAccess\Xamarin.Remote\Microsoft.CodeAnalysis.ExternalAccess.Xamarin.Remote.csproj]
    0 Warning(s)
    1 Error(s)

@CyrusNajmabadi I just merged a fix for that and will restart this build

Oh, that's appreciated. Thanks!

@sharwell Integration tests are failing, but i don't think it's me. The test is:

            SetUpEditor(@"
class Class1
{
    void Main(string[] args)
    {
        $$
    }
}");

            VisualStudio.Editor.SetUseSuggestionMode(false);

            VisualStudio.Editor.SendKeys(
                'M',
                Shift(VirtualKey.Enter));

            VisualStudio.Editor.Verify.TextContains(@"
class Class1
{
    void Main(string[] args)
    {
        Main$$
    }
}",
assertCaretPosition: true);

However, when i manually test this without my change, shift-enter takes the caret positoin and puts it here:

class Class1
{
    void Main(string[] args)
    {
        Main
            $$
    }
}

Do you know what's up here?

Oh, i see this failing for other people as well. For example: #34718

So it looks like this PR is ok. Feel free to merge when convenient @sharwell . Thanks!

         -- Cyrus

@sharwell This has all passed. can you merge in? thanks!

thanks!