[Bug]: precedence/ordering of @@access decorators is unclear

richardpark-msft commented 3 weeks ago

Describe the bug

I've got a question about @@access, when there are rules that overlap.

In our service we were attempting to make an operation public, only for the scope "go", but internal for everyone else. However, we couldn't do this:

@@access(TestThing, Access.internal);
@@access(TestThing, Access.public, "go");

This generates this type graph:

ModelTestThing
  properties:
    ModelPropertyname
      # extra stuff elided
      data:
         # NOTE that it just gave us all the same access.
         Symbol(@azure-tools/typespec-client-generator-core.access): { }

You can fix this by just ordering them differently:

@@access(TestThing, Access.public, "go");
@@access(TestThing, Access.internal);

This generates this type graph:

ModelTestThing
  properties:
    ModelPropertyname
      # extra stuff elided
      data:
         Symbol(@azure-tools/typespec-client-generator-core.access): { go: "public" }

I'm not entirely sure which behavior is the correct behavior, but I did find it surprising that the more specific directive was overridden by the least specific directive. The ordering of their declarations in the file doesn't seem like it should be a critical factor, especially given how these @@access's could be in separate files altogether.

Including @l0lawrence and @lmazuel in case I've misstated the bug.

Reproduction

Playground Link

Checklist

[X] Follow our Code of Conduct
[X] Check that this issue is about the Azure libraries for typespec. For bug in the typespec language or core libraries file it in the TypeSpec repo
[X] Check that there isn't already an issue that request the same bug to avoid creating a duplicate.
[X] The provided reproduction is a minimal reproducible example of the bug.

richardpark-msft commented 3 weeks ago

@iscai-msft, @lmazuel asked that this be assigned to you specifically.

iscai-msft commented 3 weeks ago

this seems like a bug, let me take a look at it today

richardpark-msft commented 3 weeks ago

@iscai-msft, just a note, there's another issue that @lmazuel filed about handling CSV lists inside of the 'scope' string as well. The use case was similar to what we were trying to accomplish here.

Issue: https://github.com/Azure/typespec-azure/issues/978

iscai-msft commented 2 weeks ago

@richardpark-msft after looking into it more, we don't want people to set multiple access decorators, because it gets complicated if they conflict with each other. Instead, I think the way around this is adding the ability to do a list of languages, and also adding support for ~ to exclude a language from the list. I'm going to get started on that, and close this issue in favor of #978 if you're ok with that

richardpark-msft commented 2 weeks ago

@richardpark-msft after looking into it more, we don't want people to set multiple access decorators, because it gets complicated if they conflict with each other. Instead, I think the way around this is adding the ability to do a list of languages, and also adding support for ~ to exclude a language from the list. I'm going to get started on that, and close this issue in favor of #978 if you're ok with that

I think so. If the order is ambiguous then it seems like multiple decorators should cause an outright error otherwise your result might be non-deterministic. What do you think of that?

iscai-msft commented 2 weeks ago

Yeah that's definitely why we decided originally to not allow multiple instances of either augmentation or direct decorating with @access and @usage. With the pr adding list of scopes, you should be able to do @@access("internal", "python,csharp,go,java,javascript"), which is definitely kind of long. I'm going to start another PR as well in the meantime adding support for ~, but that will require more review just because we have to determine whether we want to add support for tilde

richardpark-msft commented 2 weeks ago

Just to make sure we're talking about the same thing - in the example I gave I am applying it twice though, and there wasn't (AFAICT) an error. Should there have been? Right now it kind of appears to work, but if the resolution of that is ambiguous, depending on order, it seems like it should fail or provide some sort of warning you're putting yourself in that situation.

(the comma separated list is awesome, but I think the error condition is a separate issue)

Azure / typespec-azure