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

Reformatting a document (such as auto-save during a debugger restart) results in breakpoints pointing to wrong locations #30010

Closed
DanTup opened this issue Jul 2, 2017 · 13 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug formatting Source formatter issues verified Verification succeeded
Milestone

Comments

@DanTup
Copy link
Contributor

DanTup commented Jul 2, 2017

Given the following code:

printThingInternal() {






  
  print('Hello, world!'); // Breakpoint on this line
}

When I reformat (or more subtly, when I hit Restart in a debugging session, which automatically saves, which fires format-on-save) the breakpoint ends up pointing at a totally different piece of code.

I believe the work in #10310 was to reduce the chances of things tied to source code being moved around in reformats so I wonder whether that should include breakpoints? In this case I believe Code is recalculating my edit as just a minor delete of some newlines, so in theory it has all of the information to preserve the breakpoint at the correct location?

@vscodebot vscodebot bot added the debug Debug viewlet, configurations, breakpoints, adapter issues label Jul 2, 2017
@isidorn
Copy link
Contributor

isidorn commented Jul 3, 2017

Currently BPs are being moved as the user edits, so I believe they would move when we format as well
bpmoved

However if that is not the case then formating does some special edits which Debug is not listening on which I would find strange.
In your example you put the breakpoint on print('Hello World') and then it is moved to a wrong location? Can you also reproduce this problem in Javascript so I can investigate?

@isidorn isidorn added the info-needed Issue requires more information from poster label Jul 3, 2017
@DanTup
Copy link
Contributor Author

DanTup commented Jul 3, 2017

Weird; I'll do some more digging tonight and see if I can repro on another language or get more info.

@DanTup
Copy link
Contributor Author

DanTup commented Jul 3, 2017

@isidorn Testing with Dart, I noticed that a Format Document works fine, however the format-on-save does not (as I understand, they'll both use the same call to my extension?):

Breakpoint

I struggled to repro with JS only because formatting a JS file doesn't remove the blank lines like it does in Dart, but if you think the issue is Dart specific and know what I can do in another language that would cause lines to change I can give it a go.

@isidorn
Copy link
Contributor

isidorn commented Jul 17, 2017

@DanTup yes they should call the same format, but @jrieken can correct me if I am wrong here.
This particular issues is how editor is handling decorations on white space removal.

Can you paste your dart example code so we try to repro?

@jrieken
Copy link
Member

jrieken commented Jul 17, 2017

Yeah, all the same save-call

@DanTup
Copy link
Contributor Author

DanTup commented Jul 17, 2017

Here's an easy repro without any reliance on Dart/Dart Code.

Open an empty folder and add a file with the following content:

thing();








thing(); // Breakpoint here
thing();

// tes
// test
// test
// test
// test
// test
// test
// test
// test
// test

Create a format provider that returns the same data without the empty lines:

	provideDocumentFormattingEdits(document: TextDocument, options: FormattingOptions, token: CancellationToken): TextEdit[] {
		return [
			new TextEdit(
				new Range(document.positionAt(0), document.positionAt(document.getText().length)),
				document.getText().replace(/\n+/g, '\n').replace(/(\r\n)+/g, '\r\n')
			)
		];
	}

Now enable formatOnSave, add the breakpoint and hit Save. Then undo and choose Format Document.

This should show the issue the same as in my animation before. I'm aware that language services would be better returning a reduced edit but I'm unable to change this one (it returns the whole doc as one edit) and it works fine if you use Format Document so I think it's just a bug in formatOnSave.

LMK if you need anything more!

@isidorn
Copy link
Contributor

isidorn commented Jul 18, 2017

Thanks for the details, will investigate in the next couple of weeks.

@isidorn isidorn removed the info-needed Issue requires more information from poster label Jul 18, 2017
@isidorn isidorn added this to the August 2017 milestone Jul 18, 2017
@isidorn
Copy link
Contributor

isidorn commented Aug 4, 2017

It seems like the editor does not update those decorations in that specific case. Not sure if the combination of save / format leads to some weird timing.

However my assumption is that you could reproduce with any other editor decoration (error squigle, lightbulb...)

@isidorn isidorn removed this from the August 2017 milestone Aug 4, 2017
@isidorn isidorn removed the debug Debug viewlet, configurations, breakpoints, adapter issues label Aug 4, 2017
@isidorn isidorn assigned rebornix and unassigned isidorn Aug 4, 2017
@rebornix rebornix added bug Issue identified by VS Code Team member as probable bug editor-core Editor basic functionality labels Aug 14, 2017
@DanTup
Copy link
Contributor Author

DanTup commented Aug 17, 2017

Yep; this seems to affect other things too (for example I just saw it on some custom decorations I have - I hit Save and the code moved and they all ended up in the wrong places for a short period until my extension updated them again).

@alexdima alexdima self-assigned this Oct 26, 2017
@alexdima alexdima removed the bug Issue identified by VS Code Team member as probable bug label Oct 26, 2017
@alexdima
Copy link
Member

The editor does its very best to keep decorations positions.

But in this case, the extension sends a highly "destructive" edit. It is equivalent to select all + copy + paste. The text where the decorations sit gets entirely removed and replaced by different text.

The solution is simple: a formatter should give a minimal set of edits, preferably only edits that add or remove whitespace or newlines. That would ensure that the decorations will flow with the untouched text.

It is my recommendation to reach out to the Dart extension author(s) and kindly ask that they give as small edits as possible when formatting. This is typically very simple when one has an AST.

@alexdima alexdima added *caused-by-extension Issue identified to be caused by an extension and removed editor-core Editor basic functionality labels Oct 26, 2017
@DanTup
Copy link
Contributor Author

DanTup commented Oct 26, 2017

@alexandrudima I'm the author of the Dart extension. The reason for the large edits is that the Dart service I use provides them this way. The PR I linked to (#10310) was an implementation to make Code convert big destructive edits like this into smaller edits to reduce the burden on extension developers (this was as a fix for my issue #10133).

As mentioned - if I use Format Document I provide exactly the same edits, the decoration updates fine. It seems that Code is not doing the same thing during the automatic format-during-save operation.

I understand if you don't want to fix it, but it seems rather inconsistent for Code to be doing this for me in one place, and then not in another (it also suggests the format-during-save isn't calling the same code as a normal format document, which might be a possible source of other bugs going forwards).

@alexdima
Copy link
Member

I agree, it is inconsistent to compute more minimal edits when formatting explicitly, but not when formatting on save.

@alexdima alexdima assigned jrieken and unassigned rebornix Oct 26, 2017
@alexdima alexdima removed their assignment Oct 26, 2017
@alexdima alexdima removed the *caused-by-extension Issue identified to be caused by an extension label Oct 26, 2017
@jrieken jrieken added bug Issue identified by VS Code Team member as probable bug formatting Source formatter issues labels Oct 26, 2017
@jrieken jrieken added this to the October 2017 milestone Oct 26, 2017
jrieken added a commit that referenced this issue Oct 27, 2017
@DanTup
Copy link
Contributor Author

DanTup commented Oct 27, 2017

@jrieken Awesome, thank you! :-)

@mjbvz mjbvz added the verified Verification succeeded label Nov 2, 2017
@vscodebot vscodebot bot locked and limited conversation to collaborators Dec 11, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug formatting Source formatter issues verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

6 participants