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

Don't erase source information for inline functions #824

Open
marklam opened this issue Jan 6, 2020 · 4 comments
Open

Don't erase source information for inline functions #824

marklam opened this issue Jan 6, 2020 · 4 comments

Comments

@marklam
Copy link

@marklam marklam commented Jan 6, 2020

Include debug info sequence points for inlined functions

I propose we stop erasing the debugging sequence points for functions which have been inlined by the compiler.

The existing state is that code which has been inlined has no source file or line information. This has the effect that:

  • code coverage tools are unaware that the function has been covered, leading to incorrectly low coverage statistics (example repo)
  • automated unit test runners (such as NCover, and presumably Rider and Visual Studio Enterprise Live Unit Testing) do not re-run tests when the function implementation changes (example repo).

The second is more dangerous, and can lead to tests showing as passing when in fact recent edits should have broken them.

This screenshot shows two functionally identical tests giving different results because one implementation was inlined. Ncrunch coverage results

Pros and Cons

The advantages of making this adjustment to F# are

  • coverage reports will be more accurate, and uncovered code can be detected.
  • automated test runners will be less likely to miss tests which need to be re-run

The disadvantages of making this adjustment to F# are

  • Debugging an inlined function may be confusing (currently it's impossible)
  • Cross-assembly inlining may be a problem when the assembly came from somewhere other than the same solution.

Extra information

Estimated cost (XS, S, M, L, XL, XXL): XS -> M, depending on whether an option would be required and whether cross-assembly inlining causes a problems.

Related suggestions:
Discussion thread: dotnet/fsharp#3579

Affidavit (please submit!)

Please tick this by placing a cross in the box:

  • This is not a question (e.g. like one you might ask on stackoverflow) and I have searched stackoverflow for discussions of this issue
  • I have searched both open and closed suggestions on this site and believe this is not a duplicate
  • This is not something which has obviously "already been decided" in previous versions of F#. If you're questioning a fundamental design decision that has obviously already been taken (e.g. "Make F# untyped") then please don't submit it.

Please tick all that apply:

  • This is not a breaking change to the F# language design
  • I or my company would be willing to help implement and/or test this
@abelbraaksma

This comment has been minimized.

Copy link

@abelbraaksma abelbraaksma commented Jan 6, 2020

I've discussed this to some length with @dsyme a few years back and if I recall, he said it would be possible in principle, but far from trivial to implement.

I have searched both open and closed suggestions on this site and believe this is not a duplicate

I think there are several request out there, and maybe even some that are approved already, I'll have a look.

One tricky thing to consider is: how can a single position be both in the call site as well as on the target site of a function?

Perhaps this is easy in debug builds with noop, but in release builds I'm not so sure (and in fact, code coverage misses many more lines in release builds than only what's an inline function).

@marklam

This comment has been minimized.

Copy link
Author

@marklam marklam commented Jan 7, 2020

I generally run coverage reports in the debug build for that reason, but having at least some support in a release build could be useful for profiling tools (which is a possible 'pro' I had forgotten about in the suggestion).

@dsyme

This comment has been minimized.

Copy link
Collaborator

@dsyme dsyme commented Jan 7, 2020

I've discussed this to some length with @dsyme a few years back and if I recall, he said it would be possible in principle, but far from trivial to implement.

I need to think through the details, but in principle the core change is very very simple - remove the use of remarkExpr in Optimizer.fs and perhaps a couple of other places.

The problem may be in other places, e.g. what happens when we beta-reduce a lambda application after copying, or what goes wrong with other debugging generally.

Anyway if you want to play around with these that's where to begin I think

@dsyme

This comment has been minimized.

Copy link
Collaborator

@dsyme dsyme commented Jan 9, 2020

@abelbraaksma Do you have a record of that discussion we had?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.