reporting unnamed fields in an FCS helper function #15668
Conversation
There was a problem hiding this comment.
@psfinaki, thanks for going where none of my spell checker could ever reach!
There was a problem hiding this comment.
I have one embedded in my brains, sorry :D
…heck (just as internal implementation detail for now)
| || isNull field.Name | ||
| || System.String.Empty = field.Name | ||
|
|
||
| [<TailCall>] |
There was a problem hiding this comment.
Probably not a good idea until we move to a new released FSharp.Core which has this attribute already.
There was a problem hiding this comment.
Thanks & understood, fresh paint stuff in FSharp.Core needs at least one official release, is it how to look at it?
I used it because I was unsure my code was TailCall or StackOverflow (due to yield!), I don't know how deeply nested code is going to be passed, but I'm hopeful there is headroom in case I missed something making it non TCR-Optimized.
There was a problem hiding this comment.
Thanks & understood, fresh paint stuff in FSharp.Core needs at least one official release, is it how to look at it?
Yeah, pretty much. Because some "flavours" of FCS are built with FSharp.Core from nuget. I know @auduchinok uses it.
There was a problem hiding this comment.
Are there CI empowered people that would put a hard check on this?
I don't know how that would play, beside having a "public surface area" diff and analysis to see which members are getting exposed in the FCS codebase.
Maybe there are other ways?
Just speculating this could be a removed burden for code reviewers (if we find a robust way of doing).
There was a problem hiding this comment.
It's already there, but it's not failing CI, since I wasn't sure it was stable enough:
https://dev.azure.com/dnceng-public/public/_build/results?buildId=352682&view=logs&j=119f12ab-97c9-53f0-7ea6-00e6f97fda11&t=5269ef62-afc8-5810-682c-8c493ccd0e61
There was a problem hiding this comment.
Stellar team!
|
Hi @smoothdeveloper, are you still interested in tackling this PR? :) |
|
This is available as an analyzer at https://ionide.io/ionide-analyzers/suggestion/004.html. |
|
Closing since the same functionality is already available as an analyzer |
first bit of proof of concept for #15665
note to self: #14055 for reference of fixers dealing with DU fields.
I'm not clear if there are other things that could be checked, maybe we'd like to report bindings that are deconstructing types in function signatures; but this doesn't feel like an idiom to discourage (to me).