Should actor isolation inferred from the overrides declaration be recorded in the attributes?

I'm working on implementation for isolated synchronous deinit and as I started to write more unit tests, I realised that I don't properly understand the logic for adding attributes encoding inferred actor-isolation.

Logic seems to originate from this commit by @Douglas_Gregor. And in the original version when isolation was inferred from the overridden declaration, attributes were recorded. But in the current implementation in this case execution falls through to the final return which does not record inferred isolation.

Should isolation be recorded in this case as well? If not, why?

1 Like

We synthesize attributes (or modifiers) for nonisolated or global actor isolation, because those can be represented in the source code and it saves other clients (e.g., those parsing Swift interface files) from redoing that work. We don't synthesize any attributes for actor instance isolation, because they are not expressible in the source language and are generally cheap to recompute.

Doug

1 Like

To check my understanding - if attributes were not added, then files importing modules would take more time to compile, but result still would be correct, right?

My main question is why attributes are not synthesised when they are inferred from overriden declarations? In the original implementation it was working, but in the current implementation, it doesn't anymore. Is this an intentional change, or a bug?

Just the Swift interface files would do more work, yes. That's a one-time cost per configuration because the results of compiling Swift interface files are cached.

It was intentional. Retrieving the isolation from an override is cheap, so there's no reason to record that information in an attribute.

Doug

I think my current implementation records attributes for deinit's even if they were inferred from the overriden declaration. Should I update it to follow the logic for regular functions?

Yes, please do follow the logic for regular functions. There is no reason for deinitializers to be different here.

Doug

1 Like

I've updated PR with optimization for default actors and responded to all comments. Personally I'm very happy about it, and feel confident in merging it. What are my next steps from here?

I've run swift/test and swift/validation-test locally for macos-arm64. I don't have commit access, so I cannot trigger CI jobs. Who can I ask to start CI jobs for my PR?

Anyone else from who I should request a review?

What else do I need to do to get my evolution PR accepted and added to the review queue?

Wonderful!

I just kicked off CI.

I’ll take another look Monday. @kavon will be interested as well.

The language work group will assign a review manager and schedule a review shortly.

I’m very excited about this!

Doug

2 Likes

I think CI failures should be fixed now.

I was able to test on Linux, but had two failures which were not present on CI, in seemingly unrelated tests, might be something off with my setup. No luck in testing on Windows, but Windows failures seem to be a subset of Linux failures, so I guess it also should be fine now.

@Douglas_Gregor, need a bit of your help again.

Now, with updated rules, deinit by default does not inherit global actor isolation, but when marked with isolated, it does inherit.

@FirstActor
class Foo {
    isolated deinit {}  // FirstActor-isolated
}

How do you think it should look like in the SIL?

  • Resolved - @FirstActor deinit
  • Not resolved - isolated deinit
  • Both (don't know why) - @FirstActor isolated deinit

Note that for the source code @FirstActor isolated deinit is invalid. Rule that there can be no more than one isolation attribute still holds. And isolated deinit is invalid in the source code if there is no isolation to inherit.

1 Like