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

Refactor property validation and simplify generation #26

Merged
merged 9 commits into from
Jan 19, 2024
5 changes: 1 addition & 4 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -68,13 +68,10 @@ jobs:
linux:
name: Build and Test on Linux
runs-on: ubuntu-latest
container: swift:5.9
Copy link
Owner Author

Choose a reason for hiding this comment

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

following the advice here to avoid this kind of CI failure

steps:
- name: Checkout Repo
uses: actions/checkout@v4
- name: Setup Swift Environment
uses: swift-actions/setup-swift@v1
with:
swift-version: 5.9
- name: Build and Test Framework
run: swift test -c release --enable-code-coverage -Xswiftc -enable-testing
- name: Prepare Coverage Reports
Expand Down
2 changes: 1 addition & 1 deletion Sources/SafeDICore/Errors/FixableInstantiableError.swift
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public enum FixableInstantiableError: DiagnosticError {
public var description: String {
switch self {
case .dependencyHasTooManyAttributes:
"Dependency can have at most one of @\(Dependency.Source.instantiated), @\(Dependency.Source.received), or @\(Dependency.Source.forwarded) attached macro"
"Dependency can have at most one of @\(Dependency.Source.instantiatedRawValue), @\(Dependency.Source.receivedRawValue), or @\(Dependency.Source.forwardedRawValue) attached macro"
Copy link
Owner Author

Choose a reason for hiding this comment

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

Dependency.Source is no longer a String-backed enum, so we need to use constants for these raw values now.

case .dependencyHasInitializer:
"Dependency must not have hand-written initializer"
case .missingPublicOrOpenAttribute:
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// Distributed under the MIT License
//
// Permission is hereby granted, free of charge, to any person obtaining a copy
// of this software and associated documentation files (the "Software"), to deal
// in the Software without restriction, including without limitation the rights
// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
// copies of the Software, and to permit persons to whom the Software is
// furnished to do so, subject to the following conditions:
//
// The above copyright notice and this permission notice shall be included in all
// copies or substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
// SOFTWARE.

import SwiftSyntax

extension AttributeListSyntax.Element {

var instantiableMacro: AttributeSyntax? {
attributeIfNameEquals(InstantiableVisitor.macroName)
}

var instantiatedMacro: AttributeSyntax? {
attributeIfNameEquals(Dependency.Source.instantiatedRawValue)
}

var receivedMacro: AttributeSyntax? {
attributeIfNameEquals(Dependency.Source.receivedRawValue)
}

var forwardedMacro: AttributeSyntax? {
attributeIfNameEquals(Dependency.Source.forwardedRawValue)
}

private func attributeIfNameEquals(_ expectedName: String) -> AttributeSyntax? {
switch self {
case let .attribute(attribute):
IdentifierTypeSyntax(attribute.attributeName)?.name.text == expectedName ? attribute : nil
case .ifConfigDecl:
nil
}
}
}
42 changes: 9 additions & 33 deletions Sources/SafeDICore/Extensions/AttributeListSyntaxExtensions.swift
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,7 @@ extension AttributeListSyntax {

public var instantiableMacro: AttributeSyntax? {
guard let attribute = first(where: { element in
switch element {
case let .attribute(attribute):
return IdentifierTypeSyntax(attribute.attributeName)?.name.text == InstantiableVisitor.macroName
case .ifConfigDecl:
return false
}
element.instantiableMacro != nil
Copy link
Owner Author

Choose a reason for hiding this comment

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

I simplified our macro access while I was adding associated types to the Dependency.Source enum in order to share this code.

}) else {
return nil
}
Expand All @@ -38,12 +33,7 @@ extension AttributeListSyntax {

public var instantiatedMacro: AttributeSyntax? {
guard let attribute = first(where: { element in
switch element {
case let .attribute(attribute):
return IdentifierTypeSyntax(attribute.attributeName)?.name.text == Dependency.Source.instantiated.rawValue
case .ifConfigDecl:
return false
}
element.instantiatedMacro != nil
}) else {
return nil
}
Expand All @@ -52,38 +42,24 @@ extension AttributeListSyntax {

public var receivedMacro: AttributeSyntax? {
guard let attribute = first(where: { element in
switch element {
case let .attribute(attribute):
return IdentifierTypeSyntax(attribute.attributeName)?.name.text == Dependency.Source.received.rawValue
case .ifConfigDecl:
return false
}
element.receivedMacro != nil
}) else {
return nil
}
return AttributeSyntax(attribute)
}

public var attributedNodes: [(attribute: String, node: AttributeListSyntax.Element)] {
compactMap { element in
switch element {
case let .attribute(attribute):
guard let identifierText = IdentifierTypeSyntax(attribute.attributeName)?.name.text else {
public var dependencySources: [(source: Dependency.Source, node: AttributeListSyntax.Element)] {
compactMap {
switch $0 {
case .attribute:
guard let source = Dependency.Source(node: $0) else {
return nil
}
return (attribute: identifierText, node: element)
return (source: source, node: $0)
case .ifConfigDecl:
return nil
}
}
}

public var dependencySources: [(source: Dependency.Source, node: AttributeListSyntax.Element)] {
attributedNodes.compactMap {
guard let source = Dependency.Source.init(rawValue: $0.attribute) else {
return nil
}
return (source: source, node: $0.node)
}
}
}
137 changes: 90 additions & 47 deletions Sources/SafeDICore/Generators/DependencyTreeGenerator.swift
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ public final class DependencyTreeGenerator {
var description: String {
switch self {
case let .noInstantiableFound(typeDescription):
"No `@\(InstantiableVisitor.macroName)`-decorated type or extension found to fulfill `@\(Dependency.Source.instantiated.rawValue)`-decorated property with type `\(typeDescription.asSource)`"
"No `@\(InstantiableVisitor.macroName)`-decorated type or extension found to fulfill `@\(Dependency.Source.instantiatedRawValue)`-decorated property with type `\(typeDescription.asSource)`"
case let .unfulfillableProperties(unfulfillableProperties):
"""
The following @Received properties were never @Instantiated or @Forwarded:
Expand Down Expand Up @@ -213,38 +213,45 @@ public final class DependencyTreeGenerator {
}
})

// Populate the propertiesToInstantiate on each scope.
for scope in typeDescriptionToScopeMap.values {
var additionalPropertiesToInstantiate = [Scope.PropertyToInstantiate]()
for instantiatedDependency in scope.instantiable.instantiatedDependencies {
let instantiatedType = instantiatedDependency.asInstantiatedType
guard
let instantiable = typeDescriptionToFulfillingInstantiableMap[instantiatedType],
let instantiatedScope = typeDescriptionToScopeMap[instantiatedType]
else {
assertionFailure("Invalid state. Could not look up info for \(instantiatedType)")
continue
}
let type = instantiatedDependency.property.propertyType
switch type {
case .forwardingInstantiator:
guard !instantiable.dependencies.filter(\.isForwarded).isEmpty else {
throw DependencyTreeGeneratorError.forwardingInstantiatorInstntiableHasNoForwardedProperty(
property: instantiatedDependency.property,
instantiableWithoutForwardedProperty: instantiable)
// Populate the propertiesToGenerate on each scope.
for scope in Set(typeDescriptionToScopeMap.values) {
var propertiesToGenerate = [Scope.PropertyToGenerate]()
for dependency in scope.instantiable.dependencies {
switch dependency.source {
case .instantiated:
let instantiatedType = dependency.asInstantiatedType
guard
let instantiable = typeDescriptionToFulfillingInstantiableMap[instantiatedType],
let instantiatedScope = typeDescriptionToScopeMap[instantiatedType]
else {
assertionFailure("Invalid state. Could not look up info for \(instantiatedType)")
continue
}

case .constant, .instantiator:
break
let type = dependency.property.propertyType
switch type {
case .forwardingInstantiator:
guard !instantiable.dependencies.filter(\.isForwarded).isEmpty else {
throw DependencyTreeGeneratorError.forwardingInstantiatorInstntiableHasNoForwardedProperty(
property: dependency.property,
instantiableWithoutForwardedProperty: instantiable)
}
case .constant, .instantiator:
break
}
propertiesToGenerate.append(.instantiated(
dependency.property,
instantiatedScope
))
case let .aliased(fulfillingProperty):
propertiesToGenerate.append(.aliased(
dependency.property,
fulfilledBy: fulfillingProperty
))
case .forwarded, .received:
continue
}
additionalPropertiesToInstantiate.append(Scope.PropertyToInstantiate(
property: instantiatedDependency.property,
instantiable: instantiable,
scope: instantiatedScope,
type: type
))
}
scope.propertiesToInstantiate.append(contentsOf: additionalPropertiesToInstantiate)
scope.propertiesToGenerate.append(contentsOf: propertiesToGenerate)
}
return typeDescriptionToScopeMap
}
Expand All @@ -256,9 +263,28 @@ public final class DependencyTreeGenerator {
receivableProperties: Set<Property>,
instantiables: OrderedSet<Instantiable>
) {
let createdProperties = Set(
scope
.instantiable
.dependencies
.filter {
switch $0.source {
case .instantiated, .forwarded:
// The source is being injected into the dependency tree.
return true
case .aliased:
// This property is being re-injected into the dependency tree under a new alias.
return true
case .received:
return false
}
}
.map(\.property)
)
for receivedProperty in scope.receivedProperties {
let parentContainsProperty = receivableProperties.contains(receivedProperty)
if !parentContainsProperty {
let propertyIsCreatedAtThisScope = createdProperties.contains(receivedProperty)
if !parentContainsProperty && !propertyIsCreatedAtThisScope {
unfulfillableProperties.insert(.init(
property: receivedProperty,
instantiable: scope.instantiable,
Expand All @@ -267,21 +293,28 @@ public final class DependencyTreeGenerator {
}
}

for childScope in scope.propertiesToInstantiate.map(\.scope) {
guard !instantiables.contains(childScope.instantiable) else {
// We've previously visited this child scope.
// There is a cycle in our scope tree. Do not re-enter it.
continue
for childPropertyToGenerate in scope.propertiesToGenerate {
switch childPropertyToGenerate {
case let .instantiated(childProperty, childScope):
guard !instantiables.contains(childScope.instantiable) else {
// We've previously visited this child scope.
// There is a cycle in our scope tree. Do not re-enter it.
continue
}
var instantiables = instantiables
instantiables.insert(scope.instantiable, at: 0)

validateReceivedProperties(
on: childScope,
receivableProperties: receivableProperties
.union(scope.properties)
.removing(childProperty),
instantiables: instantiables
)

case .aliased:
break
}

var instantiables = instantiables
instantiables.insert(scope.instantiable, at: 0)

validateReceivedProperties(
on: childScope,
receivableProperties: receivableProperties.union(scope.properties),
instantiables: instantiables
)
}
}

Expand Down Expand Up @@ -314,15 +347,15 @@ extension Dependency {
switch source {
case .instantiated:
return true
case .forwarded, .received:
case .aliased, .forwarded, .received:
return false
}
}
fileprivate var isForwarded: Bool {
switch source {
case .forwarded:
return true
case .instantiated, .received:
case .aliased, .instantiated, .received:
return false
}
}
Expand All @@ -335,3 +368,13 @@ extension Array where Element == Dependency {
first(where: { !$0.isInstantiated }) == nil
}
}

// MARK: - Set

extension Set {
fileprivate func removing(_ element: Element) -> Self {
var setWithoutElement = self
setWithoutElement.remove(element)
return setWithoutElement
}
}
Loading