Apply DeMorgan's Law

[AppAppWorks]

fixed #1243

[ahoppen]

Thanks for tackling this @AppAppWorks. I have left a first set of review comments inline. Correctness-wise, I think it’s mostly missing the addition of parentheses where precedence of operators flips, eg. !(a && b || c) should get transformed to (!a || !b) && !c.

[ahoppen]

Suggested change

	case _:
	nil
	default:
	nil

[AppAppWorks]

Thanks for tackling this @AppAppWorks. I have left a first set of review comments inline. Correctness-wise, I think it’s mostly missing the addition of parentheses where precedence of operators flips, eg. !(a && b || c) should get transformed to (!a || !b) && !c.

This example involves recursive DeMorgan's Law applications that have not been implemented in this PR?
i.e. !(a && b || c) -> !(a && b) && !c -> (!a || !b) && !c

[ahoppen]

Do you mean recursive because a boolean expression is nested inside a !? I think support for these kinds of expressions is mandator for a DeMorgan code action.

[AppAppWorks]

Do you mean that whenever the code action results in any child expression like !(...), we should apply DeMorgan to such child expression recursively until no !(...) is found as a result of any DeMorgan action?

e.g. !(((a && b) || c) || d) -> !((a && b) || c) && !d -> !(a && b) && !c && !d -> (!a || !b) && !c && !d

Or do you mean that the code action will also apply recursively to all DeMorgan-able descendent expressions of the outermost DeMorgan-able expressions?

e.g. !(!(!(a && b) || c) || d) -> (!(a && b) || c) && !d -> (!a || !b || c) && !d

[ahoppen]

I would expect the former. My logic would be:

• When applying DeMorgan, add a ! to the target expression to convert (or remove it if ! already exists), then recursively DeMorgan-negate the children
• DeMorgan-negating an expression prefixed with ! will just drop the ! because that simplifies the code
• DeMorgan-negating a binary operator flips the operator and then DeMorgan-negates the operands.

With those rules, the transformation of your second example would be

!(!(!(a && b) || c) || d) -> !!(!(a && b) || c) && !d -> (!(a && b) || c) && !d

[AppAppWorks]

The latest commit is almost a complete rewrite of the code action. I've adopted a more succinct naming convention, simplified code paths, and covered more edge cases such as adding parentheses when precedence orders are flipped after conversion, and expansion on ternary expressions.

[AppAppWorks]

By the way I wonder whether there is any determining factor for creating a code action under SourceKitLSP vs a refactoring action under SwiftRefactor?

[ahoppen]

Nice. I think the recursive design is the correct one. I do wonder whether the implementation can be simplified a little bit by having fewer intermediate types. I left some comments inline but it could be that I missed something. Please let me know what you think.

[ahoppen]

Do we need the enum? Making the types fileprivate should achieve the same goal, right?

[AppAppWorks]

True, that namespace was more of part of my thought process during this rewrite.

[ahoppen]

I think it’s fine to keep the parentheses. If the user wrote them – let’s keep them. But I don’t have very strong opinions here and am happy to go with whatever is easier to implement.

[AppAppWorks]

I was thinking about the backwards conversion from a complement to which this code action has added parentheses.

[ahoppen]

Should we also include the trailing trivia of prefixExpr? There’s Trivia.merging that we use in other places to ensure that we don’t end up with duplicate spaces etc.

Suggested change

	\.leadingTrivia,
	prefixExpr.leadingTrivia + prefixExpr.expression.leadingTrivia
	prefixExpr.leadingTrivia.merging(prefixExpr.trailingTrivia).merging(prefixExpr.expression.leadingTrivia)

[ahoppen]

We have singleUnlabledExpression that extracts the expression.

Suggested change

	let only = tuple.elements.only
	let parenthesizedExpression = tuple.elements.singleUnlabledExpression

[ahoppen]

map seems like a pretty ambiguous term here but I’m struggling to find anything more meaningful. My feeling is also that the negation parameter and self duplicate some information. Just a thought: Could Negation just store the underlying PrefixOperatorExprSyntax that it was created from and then offer exprType and expr as computed properties (or stored properties if that’s better for performance)?

[AppAppWorks]

I stored the negated expr of PrefixOperatorExprSyntax directly into Negation because it's what all the intermediate data types do. It's okay to turn exprType and expr into computed properties.

[AppAppWorks]

map seems like a pretty ambiguous term here but I’m struggling to find anything more meaningful.

I had functors in mind when choosing map as the name, regrettably I also struggled with finding a better name.

[ahoppen]

Do we really have to have an enum with these two cases? Wouldn’t it be easier to just operate on ExprSyntax and have a property that looks through all parentheses? Something like

extension ExprSyntax {
  // Equivalent of the `desinglified` associated value in `single`
  var lookingThroughParentheses: ExprSyntax {
    if let tuple = self.as(TupleExprSyntax.self), 
      let underlyingExpr = tuple.singleUnlabledExpression?.lookingThroughParentheses else {
      return underlyingExpr
    }
    return self
  }

  // Equivalent of checking for the `single` case.
  var isParenthesizedExpression: Bool {
    return self.as(TupleExprSyntax.self)?.isParentheses ?? false
  }
}

[AppAppWorks]

During this rewrite I've found explicit nominal types tremendously helpful for my thought process because I've been confused for so many times by conflating SwiftSyntax types with domain specific concepts.

I'm open to remove these aids for thought at this stage ;)

[ahoppen]

While reading through the code, they confused me more than they helped me. I usually thought: Why can’t we just return the rewritten expression? Also, the map functions taking both a syntax node and one of the types you defined confused me a bit because they seemed to reason about to very similar but only related, not equivalent types simultaneously.

You did spend more time thinking about this than I did and I might be missing something, but if you could find the energy to write an implementation without the intermediate types that you define, I would very much like to see that. Maybe it will convince me that the current design is the correct one and we find the correct pieces of documentation that I would have needed to reason about it.

[AppAppWorks]

While reading through the code, they confused me more than they helped me. I usually thought: Why can’t we just return the rewritten expression? Also, the map functions taking both a syntax node and one of the types you defined confused me a bit because they seemed to reason about to very similar but only related, not equivalent types simultaneously.

It's a fair observation :) I'd leaned much towards adopting the terminology of the de Morgan's law when rewriting this code action because, for me, it'd provided a better conceptual mapping to the business logic by abstracting away the minute details of SwiftSyntax. I've struggled with and bikeshedded on naming everything, the picture didn't become clearer until I did a complete divorce from SwiftSyntax parlance.

You did spend more time thinking about this than I did and I might be missing something, but if you could find the energy to write an implementation without the intermediate types that you define, I would very much like to see that. Maybe it will convince me that the current design is the correct one and we find the correct pieces of documentation that I would have needed to reason about it.

Sure, I can still expend some time doing a minor rewrite before the start of the new school term ;)

[ahoppen]

Can you add explicit return here and in the else clause because swift-syntax needs to compile with Swift 5.8, which doesn’t have if expressions. I think the same applies to a few other methods as well.

[AppAppWorks]

Isn't SourceKitLSP compiled with Swift 5.9?

[ahoppen]

You are right. The code just uses so many swift-syntax types that I thought I was in the swift-syntax repo.

I still think that explicit returns would help readability here, though. Especially with the returned tuple.

[ahoppen]

I feel like the Operator type is a little overkill here. Wouldn’t it easier to just have a function that returns the negated operator’s TokenKind and ExprType?

[AppAppWorks]

We need something more than TokenKind here, (i.e. to tell AND or OR)

[ahoppen]

Do we need a whole type for that though? Wouldn’t it be sufficient to have

// In `NegatedPropositions`
var exprType: ExprType? {
  switch expr.operator.tokenKind {
  case .binaryOperator("|"):
    return .bitwise
  case .binaryOperator("&"):
    return .bitwise
  case .binaryOperator("||"):
    return .boolean
  case .binaryOperator("&&"):
    return .boolean
  default:
    return nil
  }
}

And where we check for operator.kind == .and also just check expr.operator.tokenKind == .binaryOperator("&") || expr.operator.tokenKind == .binaryOperator("&&").

Or alternatively, if you prefer add

extension TokenKind {
  var exprType: ExprType? {
    switch expr.operator.tokenKind {
    case .binaryOperator("|"):
      return .bitwise
    case .binaryOperator("&"):
      return .bitwise
    case .binaryOperator("||"):
      return .boolean
    case .binaryOperator("&&"):
      return .boolean
    default:
      return nil
    }
  }

  var isAnd: Bool {
    switch expr.operator.tokenKind {
    case .binaryOperator("&"):
      return .bitwise
    case .binaryOperator("&&"):
      return .boolean
    default:
      return nil
    }
  }
}

The set of and/or operators hasn’t changed since C was introduced in the 70s (or even before in other language), so I don’t think we need an abstraction layer for it.

[ahoppen]

I think this is the same in both if branches, so we should be able to extract it to the definition of leftNegated.

[ahoppen]

Do we need the NegatedPropositions type at all? It seems like all we ever do with it is to pass it into Expr.map, so instead of having the NegatedPropositions type, could we just have a function that returns the negated Expr?

[AppAppWorks]

We need to tell from negated propositions the type of its operator to determine whether we need to add extra parentheses to preserve precedence order. We can turn operator into a computed property though.

[ahoppen]

Couldn’t you do that by running the negation and then when realizing that you might need to add operators, get the transformed operator using something like expr.as(BinaryExprSyntax.self)?.operator? That way we wouldn’t have to pass around redundant state.

[AppAppWorks]

In this rewrite it was a deliberate decision to decouple domain specific concepts from Swift Syntax's parlance by creating explicit nominal types, as I thought it'd lay a good foundation for future implementations of other boolean/bitwise algebraic transformations, such as association, distribution, absorption, etc.