Allow definition of the "InvocableBy" per class member.

[epitka]

Over the years using DDD coding style, I wished many times there was a way to define a “Friend” class on the member of the class. This would make a member visible only to the class defined as a friend, similar to what we have when defining a friend assembly. There are several use cases for this, such as bi-directional relationship synchronization and/or construction of the class. Goal of this is to lock the code down and prevent logical errors especially for developers using DDD style and/or using ORMs such as EF or NHibernate.
Let take example of two classes, Order and OrderDetail. If I approach this in DDD fashion, Order is the one that should create OrderDetail, always. But in order for Order to create OrderDetail, OrderDetail must have constructor. Even if we mark constructor “internal”, it is not obvious just by looking at the code whether constructor can be invoked directly or if there is some factory, service, entity that should invoke it. So we resort to comments such as
//do not invoke directly, go through …
It would be much simpler and safer, and intent revealing, if one could write code like this (pay attention to InvocableBy attribute). Code is oversimplified just to show the concept.

public class Order
{
    public Order()
    {
        _details = new List<OrderDetail>();
    }

    public OrderDetail CreateDetail()
    {
        var detail = new OrderDetail();
        Add(detail);
        return detail;
    }
    
    private void Add(OrderDetail detail)
    {
        //bi-directional syncronization
        detail.Order = this;
        _details.Add(detail);
    }

    private readonly List<OrderDetail> _details;
    public IEnumerable<OrderDetail> Details => _details;
}

public class OrderDetail
{
    [InvocableBy(typeof(Order))]
    public OrderDetail()
    {
        
    }
  
    public Order Order { get; 
   [InvocableBy(typeof(Order))]set; }
}

Members should either be removed from IntelliSense, or warning should be shown and it should fail at compile time if member is invoked by a class that is not listed in InvocableBy. Not sure if Attribute or some other mechanism should be used.

[HaloFour]

You can write an analyzer which enforces this behavior in your projects today.

[jnm2]

This reminds me of the visibility proposal. Analyzers are okay I guess but I still wish there was some kind of idiomatic language support for things like this.

[DavidArno]

There are at least three solutions to this, using existing technologies:

1. Put ordering logic in its own project/assembly, mark OrderDetail as internal and have it implement a public IOrderDetail, which is then exposed by Order,
2. Make OrderDetail a private class within Order and expose it via the public IOrderDetail,
3. As @HaloFour says, write an analyzer that checks only classes mentioned in [InvocableBy] use OrderDetail.

Do we really need a fourth solution that requires a language change?

[epitka]

@DavidArno

Nobody is claiming that there are no solutions to this, but none of them is actually clean, and you are forced to bend your code around something that could be a part of the compilation process, and solutions actually break down really quickly. It is not even a language change, there is no new keyword, just a new attribute that compiler can look at and give a proper error. I am sure that for friend assembly there was a work around, make tests internal to your assembly, and then change compilation process to exclude does files from assembly. But for some reason compiler gives you an option to mark assembly as friend.
Now onto solutions that you are suggesting; there are few problems with them.

1. You are presuming that every project is a "greenfield" project.
2. This breaks down really quickly in a large scale projects for several reasons, in project that I worked on for insurance industry, there were over 200 entities, deeply nested, sometimes 6-7 levels. How do you suppose this would work. Have 50, 100 projects?
3. Would you nest class in a class, in a class in, what about instances where class is a child of 2-3 different classes?
4. No environment I worked in allowed multiple entities to be in a same file. It is file-per-entity, always.
5. You are forced now to use interfaces, although I still am not sure how that that would solve a problem, problem is not what is facing publicly, but within assembly. Not everybody is open to coding against interfaces.
6. Oh, and I forgot, I am not in a business of writing compiler helpers or analyzers, but in a crafting domains and solving business problems.
   I could go on, but I am sure you are getting the idea.

[Joe4evr]

No environment I worked in allowed multiple entities to be in a same file. It is class-per-entity, always.

Partials. #711 would help with this as well.

I am not in a business of writing compiler helpers or analyzers, but in a crafting domains and solving business problems.

Analyzers are designed specifically to solve such domain-specific problems, so they are tools you should learn.

[epitka]

@Joe4evr

I wanted to say "file-per-entity". I understand that analyzers would help, and certainly is a good skill to have, but seriously, percent wise, how many devs. out there even know about them?

[YaakovDavis]

#225 addresses the same concerns, using a different approach.
As said there, having a dedicated visiblity identifier, saves you typing the exact invoking members for each declaration, and also assists in analysis.
Assuming you have the same list of invokers for "families" of declarations, a named visibility is preferrable IMO.

[Pzixel]

@epitka as @DavidArno said, you already have 3 possibilities to achieve it now.

I forgot, I am not in a business of writing compiler helpers or analyzers, but in a crafting domains and solving business problems.

So you never ever write helepr methods using reflection? Because it's practically the same?

I'd like to downvote it twice if I could because this proposal just says hey, I don't want to learn existing tools, so change language as I like. Thank you. And finally, this feature gives no expressiveness but only encourage bad code design. When you want this code to be called by only several allowed types then it's just a code smell. And it leaves tons of questions: what if this methods gets called through reflection? Or by internal CLR structures? How should it work with derived classes? And if we want to allow calling it from derived classes in several assembles we specify? It's all possible through custom analyzer (even while I don't like the whole idea, but who cares), but it's really an awful idea to make is a language change. But there is already exist several approaches that can restrict access to several classes: all kinds of modifiers/indirection/inheritance/... It's just like proposals Allow multiple inheritance in C# - they just caused by incomprehension of how C# works.

I understand that analyzers would help, and certainly is a good skill to have, but seriously, percent wise, how many devs. out there even know about them?

Virtually all developers I know are aware of analyzers. Yes, they don't use them but not because they can't. Analyzers just have very narrow use area and their tasks don't require it.

[epitka]

@Pzixel
If you would have read carefully about what this is intended for, many of the questions that you posed would be irrelevant.

1."this feature gives no expressiveness but only encourage bad code design. When you want this code to be called by only several allowed types then it's just a code smell."
This is in your opinion. You care to elaborate about it, or I you just going to make a blanket statement. Maybe better not, as I don't want this to turn into what is good/bad DDD design, that is really not the point of this. I've given you reason for this, and example, as did requester in #225.
2. "what if this methods gets called through reflection?"
I could ask the same question about "protected" member, therefore protected is not needed.
3. "Or by internal CLR structures?"
Why would internal CLR be calling your domain code?
4. "How should it work with derived classes?" It would not, it would be specific to a type it is defined on.
5. "And if we want to allow calling it from derived classes in several assembles we specify? "
This does not cross assembly boundary, it is intended within assembly.

[Pzixel]

@epitka

1. Of course it is. But I have arguments thay may be considered.
2. Reflection has explicit distinction between public and non-public method. With fair reflection you get public members only.
3. Fair enough
   4-5. it makes its usage even more narrow.

Thus, you can spend 2-3 hours implementing this feature for you yourself OR you can spend several months arguing that we have to add it into the language and go to alternative v.1 if it won't be implemented.

[epitka]

@Pzixel
I had a need, that obviously I had worked around for many years, and I thought that this would be worthy enough addition, but if community thinks it is not, or it is too narrow, so be it. I am not arguing that it must be added to language, I am just voicing need that I came across, that is all. It really is not a language change, it is "just" a new attribute that VS and compiler knows about, and generates error.

[Pzixel]

@epitka it's exactly the reason why Roslyn Analyzers were created. Why not use it? Everybody can add theirs own checks - if type is referenced within same assembly, everywhere but not in this assembly, that calling type must starts with IFooBar and so on.

[iam3yal]

Is Analyzers really an option here? I mean it comes with quirks of its own, members would still show in the intellisense when it isn't appropriate, you can turn Analyzers off so it wouldn't be safe to use these members unless you have the Analyzer turned on which tells you when is the right context to use it etc...

To an extreme what you guys are saying is let's have everything public and use Analyzers to determine accessibility.

It seems like there's a weird tendency in the community where some people would propose something and then there's this other group that would suggest to replace it with extension method or an analyzer as if these tools are the silver bullet for any feature.

Anyhow, I agree with @DavidArno, option 2 is in my opinion the way to go here, sometimes you would find yourself needing a language feature because you shot yourself in the foot to being with, this isn't to say that there might be a useful examples and valid use-cases to this feature but I didn't really face one, well, maybe I had one with the provider pattern but this was long time ago. 😄

[yaakov-h]

I dunno. This proposal doesn't actually change the language, it's just asking for the compiler to throw additional errors when you contravene the rules of an attribute, which is exactly where Analyzers shine.

Perhaps if there's enough demand for such an analyser, then the LDM could consider integrating that into the language itself. However, I think it's a bit foolish to jump in cold when an analyser would solve the issue today and there's little demand (so far) for this as a language feature.

If this was actually requesting new syntax such as class OrderDetail friendof Order or similar, then perhaps a serious discussion would be appropriate, but so far it's asking to do something that you can already do today via the compiler's current extensibility points.

[jnm2]

I have to agree with @eyalsk. It hurts to have to expose some internal stuff to IntelliSense.

[yaakov-h]

I assume EditorBrowsableAttribute doesn't help here? (IIRC it gets ignored within a solution anyway, but perhaps that should be considered a bug.)

[jnm2]

@yaakov-h Precisely. Also, EditorBrowsableAttribute always feels hackish but VisibleTo does not. I'm granting another class the ability to be part of the current class, usually because I can't declare extension methods inside a non-static class.

[iam3yal]

@yaakov-h Even if it works, you're going from seeing everything to see nothing in the intellisense because there's no notion of context, also, I'm not sure whether it would work for other editors but Visual Studio.

If I have a member that only a specific a set of classes should see I want the language to help me with it, I want it to be safe^*, I want the IDE experience to feel as great as any other accessibility modifier we use today.

^* The issue with safety is how much safe, I don't know whether CLR support can be added for it which might end up being a synthetic feature or work on top of the current accessibility modifiers that we have today but that might be good enough, dunno.

Edit: Rephrased.

[epitka]

So, I tried to implement this analyzer, but quickly run into problems, probably because I don't know enough about Roslyn. This analyzer would have to perform solution wide analysis and I am not sure how to get symbols for all classes that have this attribute on the constructor. I wanted to perform analysis at SyntaxNodeAction for ObjectCreationExpression is enountered. Basically, I need to check if class being newed up is annotated with this attribute, and if it is, then check if type is newed up within a class that is defined on the attribute as the one that can invoke constructor. Initial research indicates that solution wide analysis is not something that Roslyn is good for, at least not from within analyzer. Any pointers?

[Pzixel]

@epitka I'm affraid that analyzers cannot work on solution-wide level, because they are project entities. But you probably don't need it

Here is at least one way which is pretty straightforward: you just have to create a new workspace and analyze the whole solution that all types that are referencing your class are of type declared in attribute:

MSBuildWorkspace workspace = MSBuildWorkspace.Create();
var solution = await workspace.OpenSolutionAsync(solutionPath, cancellationToken);
// here you have all information about the whole solution

If you put it in Initialize section of analyzer it takes time to create a new workspace only once and thus shouldn't take long to perform a whole analysis.

Another option is going backward: you can write an analyzer that checks that THIS class doesn't reference any class with InvocableBy attribute if it isn't listed there. For a given compilation you can reach all available types through Compilation.GlobalNamespace by iterating over all GetTypeMembers() and GetNamespaceMembers() recursively. This is not giving you all the types in the solution, but all types that are available from the current compilation (project) through all its references. But here is a problem about Fair use, because in this case it's caller who is responsible to not break the contract. However, you get a benefit: in this case, your error would be applied to a project that broke the contract. In first alternative all errors are related to the project where this attribute is declared.

[epitka]

What I would like to do is

1. At some point, maybe when solution is opened, scan solution and collect information about types that have this attribute defined either on constructor, method, or property setter.
2. Any time we have ObjectCreationExpression, SimpleAssignementExpression or InvoicationExpression, check against that list and see if type constructor, property or method has attribute declared, and if it does check that InvocableBy rule is followed.

If I could build a metadata at the start of the solution editing, or if any type is annotated with InvocableBy attribute and store that in some structure than I guess I could give real-time information about proper usage. Is there an event, or process to which I can subscribe to collect initially needed info?

[Pzixel]

@epitka

1. You always can grab information of all referencable symbols from GetTypeMember()
2. When solution is loading, it loads all projects and collect information about errors and warnings. In this case analyzers are executed. It's exactly what you need.

If I could build a metadata at the start of the solution editing, or if any type is annotated with InvocableBy attribute and store that in some structure than I guess I could give real-time information about proper usage. Is there an event, or process to which I can subscribe to collect initially needed info?

It's all on VS itself, analyzer don't have to control when user is typing or something. My expirience says that it always build up-to-date information because it's how Intellisence works.

I'l try to create a sample project.

[Pzixel]

Here it is: https://github.com/Pzixel/InvocableByAnalyzer

[jnm2]

I like VisibleTo better than InvocableBy. That way it applies to everything, including fields, nested types, nameof, etc.

[Pzixel]

@jnm2 we need analyzer to work with intellisense then. I'm not sure that we currently have API for this.

[jnm2]

@Pzixel No, it would be nice but we don't need IntelliSense to be intelligent about it. We just need an error squiggly to fail the build if you reference a field or nested type or use nameof, etc. Basically just process every identifier in the symbolic model.

[epitka]

@Pzixel: Thanks for the effort to put this analyzer together, great job.