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 debugger respect "files.exclude" or introduce a hiding mechanic #3215

Closed
Spown opened this issue Feb 21, 2016 · 29 comments
Closed

Make debugger respect "files.exclude" or introduce a hiding mechanic #3215

Spown opened this issue Feb 21, 2016 · 29 comments
Assignees
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality
Milestone

Comments

@Spown
Copy link

Spown commented Feb 21, 2016

Currently the debugger doesn't seem to care for settings.json:files.exclude. For example if I exclude **\node_modules (I assume that 99% of all errors are caused by me and things like Q, mongoose, lodash are relyible enough to not spend half of my time on clickfesting step out of these files) - the folder gets removed from the navigator, but when I traverse through breakpoints in the debugger - the node_modules files are still shown (not in the navigation column, but in the editor). I understand that files.exclude belongs to project settings, while everything debugger-related is controlled via lounch.json. These are 2 completely different entities or workflow layers that in theory shouldn't ever collide. So I guess there should be a separate config option in the lounch.json for debugger to silently jump over the excluded files. Like the --hidden flag in https://www.npmjs.com/package/node-inspector. Optionally, if so - there should be a level of control over the hiding for unhandled Exceptions:

"files.exclude": {
        "**/my_modules": false //always shown
        ,"**/libs": true //always hidden
        ,"**/node_modules": "normal" // jumps over the file if there is no breakpoint or unhandled 
        ,"**/trusted_node_modules": ["normal", "exception"] // jumps over the file if there is no breakpoint, or any other stopper except for the defined ones
    }

Also I think there should be a way to make the debugger ignore core modules either through a dedicated option or through a files.exclude key with a path-forbidden character, like "?core_module": true or something.

Thank you.

@weinand weinand added debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality labels Feb 22, 2016
@weinand weinand added this to the Backlog milestone Feb 22, 2016
@Spown
Copy link
Author

Spown commented Feb 22, 2016

yaah, as I mentioned in #3260 there is an additional inconvenience: the recently opened list gets cluttered with a ton of unwanted files...

@weinand
Copy link
Contributor

weinand commented Nov 22, 2016

The individual feature requests for node-debug and node-debug2 are:
microsoft/vscode-node-debug#16 and microsoft/vscode-chrome-debug-core#7

@weinand
Copy link
Contributor

weinand commented Nov 22, 2016

As for the syntax of the new attribute I suggest to use the same glob pattern approach as for outFiles. The name of the attribute could be blackboxFiles or blackboxCode. I don't like libraryCode because it tags code as 'library code' but that doesn't reveal what it really means.

@Spown @roblourens do you have better ideas for a good name?

@Spown
Copy link
Author

Spown commented Nov 22, 2016

Glob pattern is obvious.

Concerning the name however if I didn't know what this option would do I would think of something completely different if it would be called blackbox*. Frankly I probably wouldn't be able to make sense out of it because blackbox in my book is tightly linked with some input and output values. So I would think it somehow connected to variable inspection. I mean it's not like it is illogical, just not straightforward intuitive. Maybe something like bypass* or skip* or something like that. Also I don't like library*as well, but since it is already in use maybe introduce them as alias?..

@Eli-Goldberg
Copy link

This is really important,
what most bothers me personally is the lack of ability to make the debugger ignore exceptions in certain paths (like Q throwing random exceptions).

@weinand
Copy link
Contributor

weinand commented Nov 23, 2016

@Eli-Goldberg it will be pretty simple to include exceptions in this option as well. We could even make this the default.

@weinand
Copy link
Contributor

weinand commented Nov 23, 2016

My short list is now: hideFiles, skipFiles (because both align nicely with the existing outFiles).

@Spown @roblourens @isidorn @Eli-Goldberg opinons?

@isidorn
Copy link
Contributor

isidorn commented Nov 23, 2016

skip sounds better than hide imho, since the user can still navigate to them (thus they are not hidden). But they get skipped when stepping

@Eli-Goldberg
Copy link

Eli-Goldberg commented Nov 23, 2016

@weinand
sounds perfect, agree with @isidorn about skipFiles.
Really looking forward to not spending 50% percent of the time stepping out.

@roblourens
Copy link
Member

There is precedent for 'blackbox' in Chrome devtools, but skipFiles appeals to me immediately because it seems very clear and says exactly what it does.

How does this transfer to other strings? "Skip this file" to mark as "skipped". Stack frames show "skipped code" on frames that are skipped.

In Chrome/Node's implementation, this includes exceptions as well, so it will work that way for our extensions.

@weinand
Copy link
Contributor

weinand commented Nov 27, 2016

@roblourens what is the exact behaviour of this feature with CDP?

  • Stepping behavior: e.g. all Step commands step through code until it lands on a non skipped file,
  • Breakpoint behavior: e.g. breakpoints are ignored if they are located in skipped files,
  • Exception behavior: e.g. handled exceptions are ignored if they are located in skipped files.

@Spown
Copy link
Author

Spown commented Nov 27, 2016

maybe skipSteppingIntoFiles for maximum clarity? Just an idea. skipFiles OK with me.

@roblourens
Copy link
Member

  • Yeah, if you step in to a file/range in a file that's blackboxed, it will 'step in' until it reaches a non-blackboxed area.

  • If you set a breakpoint, it will still break at that breakpoint, and you can step manually inside that blackboxed file after that, until you 'continue' or step out of it, at which point it resumes the blackboxed behavior, which I think is useful.

  • You won't break on exceptions thrown in blackboxed scripts if you have 'break on exceptions' set.

@roblourens
Copy link
Member

And I should mention the caveat that if a blackboxed script throws an exception immediately when it's loaded, and the script is eval'ed or blackboxed by its sourcemapped source's name, then the blackboxing "probably" won't apply, as the debug adapter has to blackbox it by ID after it's loaded. This is Chrome Devtools' behavior as well.

@weinand
Copy link
Contributor

weinand commented Nov 29, 2016

@roblourens I've implemented this for node-debug and created a Test plan item.
Please close this item as soon as node-debug2 and node-debug are aligned and the test item can be used for both.

@Eli-Goldberg
Copy link

@weinand thank you so much !

@Spown
Copy link
Author

Spown commented Nov 29, 2016

@weinand I can close it right away. The further discussion can be done in #16208 until (and if) node and node2 go their separate ways. Should I?

@weinand
Copy link
Contributor

weinand commented Nov 29, 2016

@Spown since this issue has been promoted to the VS Code November plan, let's @roblourens close it.

@roblourens
Copy link
Member

Great! I didn't implement negative patterns for the glob though. I convert these to regexes and pass them to Chrome. Is there a general-purpose way to write a regex that says "matches A and does not match B"?

@Spown
Copy link
Author

Spown commented Nov 29, 2016

well, kinda... But you have to perfirm additional checks on the both negative and positive subpatterns so that they don't collide. Checking that they do not match is easy, but that the negative is partially overlaps with the positive is hard. if not impossible... Don't take my word though, that's just from the top of my head. I think normally this type of problems are devided in steps rather than encapsulated in one general purpose pattern.

@roblourens
Copy link
Member

Ok I see how I can do that using "negative lookahead". I'm inclined to print a warning if someone passes a ! glob, and ignore them, until someone complains. I don't know that I would need that level of precision in skipFiles.

@Eli-Goldberg
Copy link

IMHO, the most common use case would be a negative glob on node_modules, unless the source isnt in the root (src folder).
But if that would cause the feature to delay, the warning suggestion is probably the best one.

@roblourens
Copy link
Member

You mean if someone wants to add "skipFiles": ["node_modules", "!node_modules/thingToAllowDebugging"]? I can see that.

@Eli-Goldberg
Copy link

Yes, like having one lib throwing endless exceptions, and the other vital information.

@Spown
Copy link
Author

Spown commented Nov 30, 2016

ahm... but wouldn't "!node_modules/thingToAllowDebugging" mean "skip everything except node_modules/thingToAllowDebugging"? like absolutely everything?..

@roblourens
Copy link
Member

No, from the test item above,

Positive patterns (e.g. foo/**/*.js) add to the set of skipped files, while negative patterns (e.g. !foo/**/bar.js) subtract from that set

So I would be skipping everything in node_modules except things that match the negative patterns.

@Spown
Copy link
Author

Spown commented Nov 30, 2016

and what if you have "skipFiles": ["foo/**/*", "!bar/**/baz.js"]? If you always substract all negative patterns from all positive - that would work. but this would also mean people would think they could do "skipFiles": ["!my_code/**/*"] - in this case without any positive patterns it wouldn't do anything.

@weinand
Copy link
Contributor

weinand commented Nov 30, 2016

Yes, I've investigated this issue across various npm modules that support multiple glob patterns (or the exclude/include logic), and all of them use a model where an initial include patterns is required before exclude patterns can be applied on top of that. So just using a sole "!node_modules/thingToAllowDebugging" with various node modules results in an empty set.

@weinand weinand removed their assignment Dec 1, 2016
@roblourens
Copy link
Member

Closing this, microsoft/vscode-chrome-debug-core#135 is still pending

@egamma egamma mentioned this issue Dec 20, 2016
56 tasks
@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality
Projects
None yet
Development

No branches or pull requests

6 participants
@roblourens @weinand @isidorn @Spown @Eli-Goldberg and others