Skip to content

BestFriend on public members ought to complain #2415

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

Closed
TomFinley opened this issue Feb 5, 2019 · 4 comments · Fixed by #2434
Closed

BestFriend on public members ought to complain #2415

TomFinley opened this issue Feb 5, 2019 · 4 comments · Fixed by #2434
Assignees
Labels
nit Needs a really small fix
Milestone

Comments

@TomFinley
Copy link
Contributor

We have the best friend attribute introduced in #1520, which is great, but sometimes I or someone makes a mistake and puts it on a public class. This is harmless, has no visible effects, but is just a bit odd and definitely not intended usage.

At least for me, the procedure is this: I make something public become [BestFriend] internal, but then I realize something is a bit more complicated than I thought, and I decide to delay that specific internalization work until some other conditions are met. (E.g., in #2404, I internalized IPredictorProducing, but then realized that the problems with calibrators represented a hard barrier, so I wrote #2378 to capture those difficulties, and changed it back to public, until that is resolved.) The trouble is sometimes I leave the [BestFriend] behind. This has happened multiple times.

It might be nice, though hardly essential, to have [BestFriend] itself be analyzed to see that any usage of it is not applied to any publicly accessible thing. This is not actually essential per se, it's just a code cleanliness type of thing.

This hypothetical work would go in Microsoft.ML.InternalCodeAnalyzer.

@TomFinley
Copy link
Contributor Author

Not sure what label to give this. Not API really, since it is not user visible. Closest is nit, but the description of "nit" suggests the fix is small. (Which is a bad description. Something can be a nit without necessarily having a trivial fix.) But I guess that's the least bad description.

@TomFinley TomFinley added the nit Needs a really small fix label Feb 5, 2019
@TomFinley TomFinley self-assigned this Feb 5, 2019
@mareklinka
Copy link
Contributor

This sounds like a fun little Roslyn exercise. I'll take a swing at this.

@TomFinley
Copy link
Contributor Author

Cool, only if it sounds interesting to you though @mareklinka. But certainly learning Roslyn is very satisfying, even if you wind up not finishing this work. This is me after finishing writing the first version of RoboTom:

Unlimited power

W.r.t. testing, you can probably piggy back a lot on the test BestFriendTest which took care of a lot of the annoying plumbing w.r.t. arranging the Roslyn solution/projects, etc.

@mareklinka
Copy link
Contributor

I actually wrote my master thesis about and using Roslyn's release candidate some years back so this is not entirely new - just didn't have too many opportunities to work with it again 😄

I based the analyzer on the BestFriendTest as you suggested. It made the setup much easier.

@shauheen shauheen added this to the 0219 milestone Feb 9, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Mar 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
nit Needs a really small fix
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants