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

Make sure Sort Usings uses the current line endings #19306

Closed
KirillOsenkov opened this issue May 5, 2017 · 11 comments · Fixed by #57365
Closed

Make sure Sort Usings uses the current line endings #19306

KirillOsenkov opened this issue May 5, 2017 · 11 comments · Fixed by #57365
Assignees
Labels
Area-IDE Bug Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented
Milestone

Comments

@KirillOsenkov
Copy link
Member

I've noticed that IDE features such as Sort Usings will use a fixed line ending (Environment.NewLine?) when they need to generate a line break. This creates a problem in a file that uses LF for line endings.

The Editor has options for this: DefaultOptions.NewLineCharacterOptionId and DefaultOptions.ReplicateNewLineCharacterOptionId. Not sure whether the editor exposes these to the user in some way (and if not, we should work with the editor to expose them).

But in any case respecting these options would be nice. Ideally inferring the line endings setting from the file and using that.

@KirillOsenkov
Copy link
Member Author

@olegtk
Copy link
Contributor

olegtk commented May 5, 2017

DefaultOptions.NewLineCharacterOptionId or IEditorOperations.InsertNewLine() are the only correct ways to insert a new line. Interestingly I think Roslyn already only using these, that was done as part of editorconfig support. They are exposed to users via .editorconfig.

@Pilchie
Copy link
Member

Pilchie commented May 6, 2017

Cool. I think we just need to hook DefaultOptions.NewLineCharacterOptionId up to FormattingOptions.NewLine. @jasonmalinowski is that straightforward?

@Pilchie Pilchie added this to the 15.later milestone May 6, 2017
@Pilchie
Copy link
Member

Pilchie commented May 6, 2017

We're not likely to get to this before 15.3, but we'd welcome a PR if one appeared :-)

@jasonmalinowski
Copy link
Member

@Pilchie We have other places I'm pretty sure we use either Environment.NewLine or (worse) crlf directly. Not hard bugs, but would require a test pass.

@jinujoseph jinujoseph modified the milestones: 15.6, Unknown Nov 3, 2017
@jasonmalinowski
Copy link
Member

Curious, @KirillOsenkov are you seeing this more now with VS for Mac scenarios? Wondering if we should toss it into that bucket.

@KirillOsenkov
Copy link
Member Author

I'm seeing this during regular Roslyn dogfooding, our codebase uses \n and \r\n inserted by Roslyn are very apparent.

@jasonmalinowski
Copy link
Member

OK, good to know. @jinujoseph we may need to prioritize this now that cross-plat is actually a thing we care about.

@jinujoseph jinujoseph modified the milestones: Unknown, 15.8 Jan 22, 2018
@jinujoseph jinujoseph modified the milestones: 15.8, Unknown May 29, 2018
@jasonmalinowski jasonmalinowski removed their assignment Jan 29, 2019
@KirillOsenkov
Copy link
Member Author

// This is also serialized by the Visual Studio-specific LanguageSettingsPersister

private readonly IOption[] _supportedOptions = new IOption[]
{
FormattingOptions.UseTabs,
FormattingOptions.TabSize,
FormattingOptions.SmartIndent,
FormattingOptions.IndentationSize,
CompletionOptions.HideAdvancedMembers,
CompletionOptions.TriggerOnTyping,
SignatureHelpOptions.ShowSignatureHelp,
NavigationBarOptions.ShowNavigationBar,
BraceCompletionOptions.Enable,
};

public static async Task<DocumentOptionSet> GetDocumentOptionsWithInferredIndentationAsync(
this Document document,
bool explicitFormat,
IIndentationManagerService indentationManagerService,
CancellationToken cancellationToken)
{
var options = await document.GetOptionsAsync(cancellationToken).ConfigureAwait(false);
var text = await document.GetTextAsync(cancellationToken).ConfigureAwait(false);
var snapshot = text.FindCorrespondingEditorTextSnapshot();
if (snapshot != null)
{
indentationManagerService.GetIndentation(snapshot.TextBuffer, explicitFormat, out var convertTabsToSpaces, out var tabSize, out var indentSize);
options = options.WithChangedOption(FormattingOptions.UseTabs, !convertTabsToSpaces)
.WithChangedOption(FormattingOptions.IndentationSize, indentSize)
.WithChangedOption(FormattingOptions.TabSize, tabSize);
}
return options;
}

@sharwell
Copy link
Member

Note that Roslyn implements a formatting pass after the application of a code fix or refactoring. During this pass, elastic nodes and nodes marked with the formatter annotation will be updated to use the correct line endings. Individual code generation features should not need to worry about the line endings in the current file, as the code action infrastructure is required to account for them automatically.

@sharwell
Copy link
Member

The formatting pass already handles newlines correctly for elastic trivia. The imports organizer intentionally use non-elastic trivia to retain control over the number of newline characters appearing in the result. I'm preparing a pull request to update this feature to use the correct line ending.

sharwell added a commit to sharwell/roslyn that referenced this issue Oct 25, 2021
@sharwell sharwell self-assigned this Oct 25, 2021
@sharwell sharwell added the Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented label Oct 26, 2021
@sharwell sharwell changed the title Make sure Roslyn IDE codegen features use the current line endings Make sure Sort Usings uses the current line endings Oct 26, 2021
RikkiGibson added a commit that referenced this issue Oct 26, 2021
* Add diags to help us diagnose lightbulb issues better

* Use correct line endings when sorting imports

Fixes #19306

* Binder.CreateConversion - Report use-site diagnostics for the entire conversion hierarchy. (#57175)

* Merge pull request #57367 from dotnet/dev/rigibson/snap-17.1p1

* Compute val escape for extended property patterns (#57211)

Co-authored-by: Cyrus Najmabadi <cyrusn@microsoft.com>
Co-authored-by: Sam Harwell <Sam.Harwell@microsoft.com>
Co-authored-by: AlekseyTs <AlekseyTs@users.noreply.github.com>
Co-authored-by: CyrusNajmabadi <cyrus.najmabadi@gmail.com>
Co-authored-by: Rikki Gibson <rigibson@microsoft.com>
Co-authored-by: Alireza Habibi <habibi.alireza@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE Bug Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented
Projects
None yet
6 participants