Skip to content
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

Empty struct init #1014

Closed
wants to merge 2 commits into from
Closed

Conversation

WalterSmuts
Copy link
Contributor

Fixes #997

Suspect there is a better way not to leak abstraction and infer without saving state


let entity = Function(
isEmptyInit: program.ast[d].isInit && layout.properties.isEmpty,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there's much to win memoizing this test. We can compute it in Module+NormalizeObjectStates.swift and save us the trouble of storing an otherwise needless property.

You can get the declaration from which an IR function was lowered by looking at FunctionID.value and you should be able to build the abstract layout of the receiver in the IR pass. If I'm not mistaken, the type of the first argument of a lowered initializer must be self.

So we can write something like this (in the IR pass):

if case .lowered(let d) = f.value, let i = InitializerDecl.ID(d) {
  if !program.ast[i].isMemberwise {
    let l = AbstractTypeLayout(
      of: self[f].inputs[0].type.bareType,
      definedIn: program)
    if l.properties.isEmpty { ... }
  }
}

I would probably implement this code in a function of IR.Module, like isEmptyInitializer(_:).

Copy link
Contributor Author

@WalterSmuts WalterSmuts Sep 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can get the declaration from which an IR function was lowered by looking at FunctionID.value

These are the things that really slow me down 🙈 Spelunking through a new code-base to do the necessary conversions. I know there must be a path but it takes a while to build up the context to find it easily. This is exactly what I intended :) Just couldn't find it and opted for the convoluted workaround instead

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fully understandable. I'd like to setup live and/or recorded code walks to speed up this process.

Copy link
Contributor

@kyouko-taiga kyouko-taiga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should avoid storing a property in IR.Function.

Another approach you may consider is to have the emitter insert make_state [initialized] %self before the return instructions of an empty initializer. The instruction will "convince" the borrow checker that self is initialized and it will have no effect if self is initialized explicitly.

@WalterSmuts
Copy link
Contributor Author

Weird that this doesn't compile on the CI 🤔 I've successfully recompiled twice now locally and verified I am on the same commit be6d4684. I nuked my .build to ensure I wasn't using a stale cached artefact. Weird. Suspect it may be the CI system's cache...

@kyouko-taiga
Copy link
Contributor

Will try to build on my system.

@kyouko-taiga
Copy link
Contributor

kyouko-taiga commented Sep 15, 2023

I get the same error as CI on my system:

/Developer/hc/Sources/IR/Module.swift:297:7: error: missing argument for parameter 'isEmptyInit' in call
      isSubscript: false,
      ^
      isEmptyInit: <#Bool#>, 

I suspect the problem is due to an auto-merge with main that now contains new calls to Function.init, which your PR changes. Try to merge main into your local branch.

@WalterSmuts
Copy link
Contributor Author

WalterSmuts commented Sep 16, 2023

Another approach you may consider is to have the emitter insert make_state [initialized] %self before the return instructions of an empty initializer. The instruction will "convince" the borrow checker that self is initialized and it will have no effect if self is initialized explicitly.

This approach seems cleaner. Is there any downsides to emitting the make_state [initialized] %self directive? Perhaps it has some other side effects?

Carving out a special case for empty initializers when doing initialization checking looks a bit hacky. I guess the question is whether it's more hacky than the special case for injserting the make_state [initialized] %self directive. Will check both (already pushed the first option).

@kyouko-taiga
Copy link
Contributor

This approach seems cleaner. Is there any downsides to emitting the make_state [initialized] %self directive? Perhaps it has some other side effects?

Not really. mark_state is a ghost instruction. It has no operational semantics. The only requirement is that we should use it carefully because it's essentially a way to tell the borrow checker "trust me, I know this think is [un]initialized".

Carving out a special case for empty initializers when doing initialization checking looks a bit hacky.

Unfortunately I think we need to carve out an exception. The rationale is that in other places we do want to track the logical initialization of empty objects.

public fun main() {
  var x: Void
  print(x) // error: 'x' is not initialized
}

Will check both (already pushed the first option).

Okay, let me know if you need any help.

@WalterSmuts
Copy link
Contributor Author

Closed in favour of #1016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Structures without elements produce error in init function
2 participants