feat: enable Nullable<T> inspection for complex types (Guid?, DateTime?, etc.)

[GustavEikaas]

My attempt at fixing #213. Keep in mind that I usually never code in c++.
In the case where my implementation has bugs or unhandled cases, feel free to tag me and describe what is missing. I'm happy to test and fix it.
I did test the code and it works for the cases described in #213.

Fixes #213

Test code

    Guid? x = Guid.NewGuid();
    Guid? y = null;
    Guid? z = new Guid();
    DateTime? dt1 = DateTime.Now;
    DateTime? dt2 = null;
    TimeSpan? ts1 = TimeSpan.FromSeconds(42);
    TimeSpan? ts2 = null;
    int? i1 = 42;
    int? i2 = null;
    bool? b1 = true;
    bool? b2 = null;
    double? d1 = 3.14;
    double? d2 = null;
    DayOfWeek? day1 = DayOfWeek.Monday;
    DayOfWeek? day2 = null;
    decimal? dec1 = 123.456m;
    decimal? dec2 = null;

Background

I maintain https://github.com/GustavEikaas/easy-dotnet.nvim which relies on netcoredbg for debugging using nvim-dap and I use it daily. It is annoying to use repl window every time I see the type, but with variablesReference = 0.

[GustavEikaas]

For some reason when building from master branch I keep getting this error. I had to just comment out the code to get it to run properly

netcoredbg: /home/gustav/repo/netcoredbg/src/metadata/modules_sources.h:68: bool netcoredbg::method_data_t::NestedInto(const method_data_t &) const: Assertion `startLine != other.startLine || startColumn != other.startColumn' failed.

[gbalykov]

Thanks for your contribution! We'll take a look.

For some reason when building from master branch I keep getting this error

As a workaround you can comment it our for now. We have a fix for this is internal repo, will be available here on next sync.

[GustavEikaas]

Any updates on this? Feel free to tell me if it's no good and I will close the PR

[viewizard]

Apologies for the delay. This looks good to me, and I’ll test these changes at work on Monday. Unfortunately, we're having issues with our CI right now, so I'm forced to run all the netcoredbg tests manually, and I usually just don't have time for this (T_T).

[GustavEikaas]

No worries, thank you for taking a look! I was just afraid it was bad and nobody had the heart to tell me

I am considering looking into implementing column number support for breakpoints next. In my plugin im able to validate if there are multiple breakpoint candidates on the same line using roslyn. Thoughts on that?

[viewizard]

I am considering looking into implementing column number support for breakpoints next. In my plugin im able to validate if there are multiple breakpoint candidates on the same line using roslyn. Thoughts on that?

At this time, netcoredbg should be extensively refactored to allow setting source breakpoints with both line and column information. The internal logic currently works differently: when the user provides a line number, the debugger searches for the sequence point on that line (for multi-line code) or the first sequence point that follows it, completely ignoring the column number. Specifically, when a column number is provided, the debugger must handle cases where multiple statements appear on a single line, such as int i = 0; i++; i--; i = 5;.

[GustavEikaas]

yeah I was actually just looking at the code for that. I figured out how to pass the col down from the DAP request all the way to the C# SymbolReader, but I ran into exactly what you mentioned with the i++; stuff.

I noticed EnableOneICorBreakpointForLine basically forces only one active breakpoint per line and the map is keyed by line number right now.

Should I just use the column to pick the right IL offset (which fixes the linq lambdas) but keep the 1 BP per line rule? Or do you want me to try and rewrite the mapping to key off the IL offset so we can actually have multiple active on the same line? Im cool with either but the first one is definitely easier for a c++ noob like me lol.

I am doing this for neovim which doesnt really support multiple bps per line AFAIK. Especially in terms of rendering so 1 per line works best for me. Thoughts?

[GustavEikaas]

I'm also assuming its okay to do the same matching logic with col numbers as line numbers. if user sends 6 and 5 is valid it doesnt throw it just picks 5?

[viewizard]

Should I just use the column to pick the right IL offset (which fixes the linq lambdas) but keep the 1 BP per line rule? Or do you want me to try and rewrite the mapping to key off the IL offset so we can actually have multiple active on the same line? Im cool with either but the first one is definitely easier for a c++ noob like me lol.

The key point is that these changes will not break the existing logic. The ability to "use the column to pick the right IL offset" could likely be implemented easily with the current codebase and would be more useful. By default, it should use column 0 (which matches the current behavior exactly). Why would you need to set four breakpoints on a single line like int i = 0; i++; i--; i = 5; when you could just set one and step three times?

I am doing this for neovim which doesnt really support multiple bps per line AFAIK. Especially in terms of rendering so 1 per line works best for me. Thoughts?

I don't know of any IDE that supports this feature. That's exactly why it hasn't made it onto our TODO list. :)

I'm also assuming its okay to do the same matching logic with col numbers as line numbers. if user sends 6 and 5 is valid it doesnt throw it just picks 5?

Sure. The debugger should check whether the given line/column is inside a valid sequence point - if so, it sets the breakpoint there. If it falls outside all sequence points, it should automatically find the next valid one in the code.

[viewizard]

I tested these changes, all tests pass. I created a PR in the internal dev repo. I'll wait for our internal CI to be fixed, which will allow merging these changes into the internal dev repo. All changes will be available at the next public repo sync.

[GustavEikaas]

Awesome, thank you so much!