microsoft / AL

Home of the Dynamics 365 Business Central AL Language extension for Visual Studio Code. Used to track issues regarding the latest version of the AL compiler and developer tools available in the Visual Studio Code Marketplace or as part of the AL Developer Preview builds for Dynamics 365 Business Central.
MIT License
728 stars 241 forks source link

Compiler Bug: Inconsistent accessibility is not enforced #7614

Closed hemisphera closed 6 months ago

hemisphera commented 8 months ago

1. Describe the bug Inconsistent accessibility errors are not enforced compile-time, something like CS0051 is checked for example in in C#.

An example would be an internal table that is exposed to the public via event publisher. Everyone can subscribe to a (normal integration or business) event, and therefore all parameters that this event exposes, must also be exposable.

The problem is that this can lead to potentially exposing things that one really didn't want to expose and - worse - the consumer obviously can't consume. Furthermore: one is then not even able to rectify because doing so would be a breaking change. So basically a whole lot of noise and mess and clean-up required of something that IMHO shouldn't event be allowed to compile.

2. To Reproduce

Create a random table

table 50100 ARandomTable
{
  Access = Internal;
  // do stuff
}

Create random codeunit with an event publisher.

codeunit 50100 ARandomCodeunit
{

    [IntegrationEvent(false, false)]
    local procedure DereBeEventsMon(var stuff: Record "ARandomTable")
    begin
    end;

}

3. Expected behavior I would expect a compilation error as this violates accessibility constraints.

4. Actual behavior It compiles just fine.

5. Versions:

BazookaMusic commented 8 months ago

Maybe I don't understand the scenario completely, but I think this is valid code. In order to subscribe to DereBeEventsMon, I'd need to make a subscriber procedure with the same event signature, which as you said, I cannot write if I don't have access to "ARandomTable" in another app. So other apps cannot subscribe to this event.

However, we also have the internalsVisibleTo property for the app.json which lets me expose internal members to another app (eg. a unit test app). Then I could subscribe to this event from say a unit test application to test some stuff.

Thus I don't think your example violates the accessibility constraints.

If there is actually a way to make a subscriber function in an app without access to ARandomTable, then we need to consider that. Is that the case?

hemisphera commented 8 months ago

Yes, the internalsVisibleTo is a valid concern. However, as you said, it's mostly used for unit testing apps - at least I think it should. As such you could easily limit the compiler error to arise only if no internalsVisibleTo has been specified. Of course you cannot know what app has what access, but when there is no app at all in that property, then my code example makes no sense.

Also: we have InternalEvent events. If I really intended my event to be consumed by an 'internals-enabled' app, I'd rather should be using that instead of a 'public' event, where internals are getting exposure. That would be way cleaner.

I get your point. But one of the biggest issues we face is removing something that was exposed by error because of the various breaking change policies. This would help in mitigation.

ChrisBlankDe commented 8 months ago

Another example for this would be an public table containing an public field of an internal enum type. I would be very happy if we could check this at compile time and issue at least a warning

JesperSchulz commented 6 months ago

The fix for this issue has been checked in to the master branch. It will be available in the bcinsider.azurecr.io/bcsandbox-master Docker image starting from platform build number 24.0.15860.0 and VS Code Extension Version 13.0.935369.

If you don’t have access to these images you need to become part of the Ready2Go program: aka.ms/readytogo

For more details on code branches and docker images please read: https://blogs.msdn.microsoft.com/nav/2018/05/03/al-developer-previews-multiple-releases-and-github/ https://freddysblog.com/2020/06/25/working-with-artifacts/