ronantakizawa/codereview-bench huggingface dataset:
Repository: apple/swift
File: test/ClangImporter/enum-closed-raw-value.swift (Swift)

Code before:
// RUN: %empty-directory(%t)
// RUN: %target-build-swift %s -import-objc-header %S/Inputs/enum-closed-raw-value.h -o %t/a.out
// RUN: %target-codesign %t/a.out
// RUN: %target-run %t/a.out | %FileCheck %s

// REQUIRES: executable_test

// Test that NS_CLOSED_ENUM (frozen enum) only accepts valid raw values
// and returns nil for invalid values in init?(rawValue:)

func testIntEnum() {
  // Valid values should succeed
  if let zero = IntEnum(rawValue: 0) {
    print("IntEnum(0): valid - \(zero == .zero)")
    // CHECK: IntEnum(0): valid - true
  } else {
    print("IntEnum(0): ERROR - should not be nil")
  }
  
  if let one = IntEnum(rawValue: 1) {
    print("IntEnum(1): valid - \(one == .one)")
    // CHECK: IntEnum(1): valid - true
  } else {
    print("IntEnum(1): ERROR - should not be nil")
  }
  
  // Invalid values should return nil
  if let invalid = IntEnum(rawValue: 100) {
    print("IntEnum(100): ERROR - should be nil, got \(invalid)")

Review comment (suggestion):
Can you please use `StdlibUnittest` and its `expectEqual()` functions, rather than pipe the output to `FileCheck`? See `test/Interop/Cxx/enum/hashable-enums.swift` for an example.

Code after:
// RUN: %target-run-simple-swift(-import-objc-header %S/Inputs/enum-closed-raw-value.h)
// RUN: %target-run-simple-swift(-cxx-interoperability-mode=default -import-objc-header %S/Inputs/enum-closed-raw-value.h)

// REQUIRES: executable_test

import StdlibUnittest

var EnumClosedRawValueTestSuite = TestSuite("NS_CLOSED_ENUM raw value validation")

EnumClosedRawValueTestSuite.test("IntEnum: valid raw values") {
  let zero = IntEnum(rawValue: 0)
  expectNotNil(zero)
  expectEqual(zero!, .zero)

  let one = IntEnum(rawValue: 1)
  expectNotNil(one)
  expectEqual(one!, .one)
}

EnumClosedRawValueTestSuite.test("IntEnum: invalid raw values return nil") {
  // This should pass with the fix - invalid values return nil
  expectNil(IntEnum(rawValue: 100))
  expectNil(IntEnum(rawValue: -1))
  expectNil(IntEnum(rawValue: 2))
}

EnumClosedRawValueTestSuite.test("NonContiguousEnum: valid raw values") {
  let first = NonContiguousEnum(rawValue: 10)
  expectNotNil(first)
Repository: apple/swift
File: test/embedded/dependencies-print.swift (Swift)

Code before:
// RUN: %empty-directory(%t)
// RUN: split-file %s %t
// RUN: %target-swift-frontend -enable-experimental-feature Embedded -no-allocations %t/test.swift -c -o %t/a.o

// RUN: %llvm-nm --undefined-only --format=just-symbols %t/a.o | sort | tee %t/actual-dependencies.txt

// RUN: %if OS=linux-gnu %{ sort %t/allowed-dependencies_linux.txt -o %t/allowed-dependencies_linux.txt %} %else %{ sort %t/allowed-dependencies_macos.txt -o %t/allowed-dependencies_macos.txt %}
// RUN: %if OS=linux-gnu %{ comm -13 %t/allowed-dependencies_linux.txt %t/actual-dependencies.txt > %t/extra.txt %} %else %{ comm -13 %t/allowed-dependencies_macos.txt %t/actual-dependencies.txt > %t/extra.txt %}
// RUN: test ! -s %t/extra.txt

//--- allowed-dependencies_macos.txt
___stack_chk_fail
___stack_chk_guard
___divti3
___modti3
_memmove
_memset
_putchar

//--- allowed-dependencies_linux.txt
__stack_chk_fail
__stack_chk_guard
__divti3
__modti3
memmove
memset
putchar

//--- test.swift
// DEP: ___stack_chk_fail
// DEP: ___stack_chk_guard
// DEP: ___divti3

Review comment (bug):
We could just sort these manually and skip this step:

```
//--- allowed-dependencies_macos.txt
___divti3
___modti3
___stack_chk_fail
___stack_chk_guard
_memmove
_memset
_putchar

//--- allowed-dependencies_linux.txt
__divti3
__modti3
__stack_chk_fail
__stack_chk_guard
memmove
memset
putchar
```

Code after:
// RUN: %empty-directory(%t)
// RUN: split-file %s %t
// RUN: %target-swift-frontend -enable-experimental-feature Embedded -no-allocations %t/test.swift -c -o %t/a.o

// RUN: %llvm-nm --undefined-only --format=just-symbols %t/a.o | sort | tee %t/actual-dependencies.txt

// RUN: %if OS=linux-gnu %{ comm -13 %t/allowed-dependencies_linux.txt %t/actual-dependencies.txt > %t/extra.txt %} %else %{ comm -13 %t/allowed-dependencies_macos.txt %t/actual-dependencies.txt > %t/extra.txt %}
// RUN: test ! -s %t/extra.txt

//--- allowed-dependencies_macos.txt
___divti3
___modti3
___stack_chk_fail
___stack_chk_guard
_memmove
_memset
_putchar

//--- allowed-dependencies_linux.txt
__divti3
__modti3
__stack_chk_fail
__stack_chk_guard
memmove
memset
putchar
//--- test.swift
// RUN: %target-clang -x c -c %S/Inputs/print.c -o %t/print.o
// RUN: %target-clang %t/a.o %t/print.o -o %t/a.out
// RUN: %target-run %t/a.out | %FileCheck %s

// REQUIRES: swift_in_compiler
Repository: apple/swift
File: stdlib/private/SwiftPrivateThreadExtras/ThreadBarriers.swift (Swift)

Code before:
    }
    ReleaseSRWLockExclusive(barrier.pointee.mutex!)
#elseif os(WASI)
  // WASI environment has a only single thread
#else
    if pthread_cond_wait(barrier.pointee.cond!, mutex) != 0 {
      return -1
    }
    shouldUnlockMutex = false
    if pthread_mutex_unlock(mutex) != 0 {
      return -1
    }
#endif
    return 0
  } else {
    // Reset thread count.
    barrier.pointee.numThreadsWaiting = 0

    // Wake up all threads.
#if os(Windows)
    WakeAllConditionVariable(barrier.pointee.cond!)
    ReleaseSRWLockExclusive(barrier.pointee.mutex!)
#elseif os(WASI)
  // WASI environment has a only single thread
#else
    if pthread_cond_broadcast(barrier.pointee.cond!, mutex) != 0 {
      return -1
    }
    shouldUnlockMutex = false
    if pthread_mutex_unlock(mutex) != 0 {
      return -1
    }
#endif
    return _stdlib_THREAD_BARRIER_SERIAL_THREAD
  }
}


Review comment (bug):
This additional parameter appears to be wrong:
```
11:26:29  185 | #else
11:26:29  186 |     if pthread_cond_broadcast(barrier.pointee.cond!, mutex) != 0 {
11:26:29      |                                                      `- error: extra argument in call
11:26:29  187 |       return -1
11:26:29  188 |     }
```

Code after:
    }
    ReleaseSRWLockExclusive(barrier.pointee.mutex!)
#elseif os(WASI)
  // WASI environment has a only single thread
#else
    if pthread_cond_wait(barrier.pointee.cond!, mutex) != 0 {
      return -1
    }
    shouldUnlockMutex = false
    if pthread_mutex_unlock(mutex) != 0 {
      return -1
    }
#endif
    return 0
  } else {
    // Reset thread count.
    barrier.pointee.numThreadsWaiting = 0

    // Wake up all threads.
#if os(Windows)
    WakeAllConditionVariable(barrier.pointee.cond!)
    ReleaseSRWLockExclusive(barrier.pointee.mutex!)
#elseif os(WASI)
  // WASI environment has a only single thread
#else
    if pthread_cond_broadcast(barrier.pointee.cond!) != 0 {
      return -1
    }
    shouldUnlockMutex = false
    if pthread_mutex_unlock(mutex) != 0 {
      return -1
    }
#endif
    return _stdlib_THREAD_BARRIER_SERIAL_THREAD
  }
}
Repository: apple/swift
File: SwiftCompilerSources/Sources/SIL/Context.swift (Swift)

Code before:
    // We rely that after deleting the instruction its operands have no users.
    // Therefore `salvageDebugInfo` must be turned off because we cannot insert debug_value instructions.
    erase(instruction: inst, salvageDebugInfo: false)
  }

  public func erase<S: Sequence>(instructions: S) where S.Element: Instruction {
    for inst in instructions {
      erase(instruction: inst)
    }
  }

  public func erase(instructionIncludingDebugUses inst: Instruction) {
    precondition(inst.results.allSatisfy { $0.uses.ignoreDebugUses.isEmpty })
    salvageDebugInfo(of: inst)
    erase(instructionIncludingAllUsers: inst)
  }

  public func erase(block: BasicBlock) {
    _bridged.eraseBlock(block.bridged)
  }

  /// Transfer debug info associated with (the result of) this instruction to a
  /// new `debug_value` instruction before this instruction is deleted.
  public func salvageDebugInfo(of instruction: Instruction) {
    BridgedContext.salvageDebugInfo(instruction.bridged)
    notifyInstructionsChanged()
  }
}


Review comment (bug):
We should only notify instructions changed when a new debug_value was actually inserted. We don't want to do this if there was no debug_info to salvage. 

I think my TODO refers to `CanonicalizeInstruction::notifyNewInstruction`. The SwiftCompilerSources bridging API was added later by @eeckstein. You can probably just delete that TODO for two reasons
- I don't think Canonicalization calls back to SwiftCompilerSources and we're not adding new C++ canonicalization
- I can't imagine any analysis would actually need to know whether a new `debug_value` instruction was created

More generally, do we care at all if `salvageDebugInfo` invalidates or notifies the context?
@eeckstein what do you think and what's the best strategy for notifying the context about new `debug_value`? I'm fine either ignoring the new instruction, or returning a `bool` if any change was made and letting the bridging API call notifyInstructionsChanged conditionally.

Code after:
    // We rely that after deleting the instruction its operands have no users.
    // Therefore `salvageDebugInfo` must be turned off because we cannot insert debug_value instructions.
    erase(instruction: inst, salvageDebugInfo: false)
  }

  public func erase<S: Sequence>(instructions: S) where S.Element: Instruction {
    for inst in instructions {
      erase(instruction: inst)
    }
  }

  public func erase(instructionIncludingDebugUses inst: Instruction) {
    precondition(inst.results.allSatisfy { $0.uses.ignoreDebugUses.isEmpty })
    salvageDebugInfo(of: inst)
    erase(instructionIncludingAllUsers: inst)
  }

  public func erase(block: BasicBlock) {
    _bridged.eraseBlock(block.bridged)
  }

  /// Transfer debug info associated with (the result of) `instruction` to a
  /// new `debug_value` instruction before `instruction` is deleted.
  public func salvageDebugInfo(of instruction: Instruction) {
    BridgedContext.salvageDebugInfo(instruction.bridged)
  }
}
Repository: apple/swift
File: test/Constraints/issue-85942.swift (Swift)

Code before:
// expected-error@1 {{new Swift parser generated errors for code that C++ parser accepted}}
// RUN: %target-typecheck-verify-swift

// https://github.com/swiftlang/swift/issues/85942
// Verify that we do not crash when a KeyPath is used in a map with missing arguments.

func f(buttonsStr: [Substring]) {
    // expected-error@+3 {{unexpected code '(separator: ",").compactMap(Int.init)' in function call}}
    // expected-error@+2 {{key path cannot refer to instance method 'compactMap'}}
    // expected-error@+1 {{cannot convert value of type '() -> Int' to expected argument type '(ArraySlice<Character>) throws -> Int?'}}
    let _: [[Int]] = buttonsStr.map(\.split(separator: ",").compactMap(Int.init))
}


Review comment (suggestion):
One more thing - could you please use `// RUN: %target-typecheck-verify-swift -disable-experimental-parser-round-trip` here? It would remove all of this round-trip diagnostics like `new Swift parser generated` and `unexpected code`.

Code after:
// RUN: %target-typecheck-verify-swift -disable-experimental-parser-round-trip

// https://github.com/swiftlang/swift/issues/85942
// Verify that we do not crash when a KeyPath is used in a map with missing arguments.

func f(buttonsStr: [Substring]) {
    // expected-error@+2 {{key path cannot refer to instance method 'compactMap'}}
    // expected-error@+1 {{cannot convert value of type '() -> Int' to expected argument type '(ArraySlice<Character>) throws -> Int?'}}
    let _: [[Int]] = buttonsStr.map(\.split(separator: ",").compactMap(Int.init))
}
Repository: apple/swift
File: lib/SIL/IR/SILType.cpp (C++)

Code before:
    SILModule &module = F.getModule();
    if (structDecl->isResilient(module.getSwiftModule(),
                                F.getResilienceExpansion())) {
      return false;
    }

    TypeExpansionContext typeEx = F.getTypeExpansionContext();
    for (VarDecl *field : structDecl->getStoredProperties()) {
      if (!getFieldType(field, module, typeEx).isEmpty(F))
        return false;
    }
    return true;
  }

  if (auto bfa = getAs<BuiltinFixedArrayType>()) {
    if (auto size = bfa->getFixedInhabitedSize()) {
      return size == 0;
    }
  }

  return false;
}

bool SILType::isEmptyTuple(const SILFunction &F) const {
  // Infinite types are never empty.
  if (F.getTypeProperties(*this).isInfinite()) {
    return false;
  }

  if (auto tupleTy = getAs<TupleType>()) {
    // A tuple is empty if it either has no elements or if all elements are
    // empty.
    for (unsigned idx = 0, num = tupleTy->getNumElements(); idx < num; ++idx) {
      if (!getTupleElementType(idx).isEmptyTuple(F))
        return false;
    }
    return true;
  }

  return false;
}

bool SILType::isReferenceCounted(SILModule &M) const {
  return M.Types.getTypeLowering(*this,
                                 TypeExpansionContext::minimal())
    .isReferenceCounted();
}

bool SILType::isReferenceCounted(SILFunction *f) const {
  return isReferenceCounted(f->getModule());
}

Review comment (refactor):
isInfinite() means "infinite by exploring stored properties", not tuple elements, so there's no danger of infinite recursion below. You can remove this check

Code after:
    SILModule &module = F.getModule();
    if (structDecl->isResilient(module.getSwiftModule(),
                                F.getResilienceExpansion())) {
      return false;
    }

    TypeExpansionContext typeEx = F.getTypeExpansionContext();
    for (VarDecl *field : structDecl->getStoredProperties()) {
      if (!getFieldType(field, module, typeEx).isEmpty(F))
        return false;
    }
    return true;
  }

  if (auto bfa = getAs<BuiltinFixedArrayType>()) {
    if (auto size = bfa->getFixedInhabitedSize()) {
      return size == 0;
    }
  }

  return false;
}

bool SILType::isEmptyTuple(const SILFunction &F) const {
  if (auto tupleTy = getAs<TupleType>()) {
    // A tuple is empty if it either has no elements or if all elements are
    // empty.
    for (unsigned idx = 0, num = tupleTy->getNumElements(); idx < num; ++idx) {
      if (!getTupleElementType(idx).isEmptyTuple(F))
        return false;
    }
    return true;
  }

  return false;
}

bool SILType::isReferenceCounted(SILModule &M) const {
  return M.Types.getTypeLowering(*this,
                                 TypeExpansionContext::minimal())
    .isReferenceCounted();
}

bool SILType::isReferenceCounted(SILFunction *f) const {
  return isReferenceCounted(f->getModule());
}

bool SILType::isNoReturnFunction(SILModule &M,
                                 TypeExpansionContext context) const {
  if (auto funcTy = dyn_cast<SILFunctionType>(getASTType()))
    return funcTy->isNoReturnFunction(M, context);
Repository: apple/swift
File: test/SourceKit/Misc/match-module-cache-with-compiler.swift (Swift)

Code before:
// This test ensures a certain set of arguments allows both compiler and sourcekitd invocations to share the same module cache.
// NOTE: Do not change this test without a review from @akyrtzi

// UNSUPPORTED: OS=windows-msvc

// RUN: %empty-directory(%t)

// DEFINE: %{args} = \
// DEFINE:   -module-name themod \
// DEFINE:   -module-cache-path %t/mcp \
// DEFINE:   -sdk %sdk \
// DEFINE:   -swift-version 5 \
// DEFINE:   -I %S/Inputs/mymod \
// DEFINE:   -Xfrontend -experimental-allow-module-with-compiler-errors \
// DEFINE:   -Xcc -D__INDEX_BUILD__=1 -D__INDEX_BUILD__ \
// DEFINE:   -Xcc -Xclang -Xcc -fallow-pcm-with-compiler-errors \
// DEFINE:   -Xcc -Xclang -Xcc -fmodule-format=raw \
// DEFINE:   -Xcc -Xclang -Xcc -detailed-preprocessing-record \
// DEFINE:   %s

// RUN: %swiftc_driver -emit-module -emit-module-path themod.swiftmodule -Xfrontend -experimental-skip-all-function-bodies  %{args}
// RUN: %swiftc_driver -index-file -index-file-path %s -index-store-path %t/idx -index-ignore-system-modules %{args}
// RUN: %sourcekitd-test -req=sema %s -- %{args}
// RUN: %find_files %t%{fs-sep}mcp 'mymod-*.pcm' | count 1

import mymod


Review comment (suggestion):
```suggestion
// https://github.com/swiftlang/swift/issues/86930
// UNSUPPORTED: OS=windows-msvc
```

Code after:
// This test ensures a certain set of arguments allows both compiler and sourcekitd invocations to share the same module cache.
// NOTE: Do not change this test without a review from @akyrtzi

// https://github.com/swiftlang/swift/issues/86930
// UNSUPPORTED: OS=windows-msvc

// RUN: %empty-directory(%t)

// DEFINE: %{args} = \
// DEFINE:   -module-name themod \
// DEFINE:   -module-cache-path %t/mcp \
// DEFINE:   -sdk %sdk \
// DEFINE:   -swift-version 5 \
// DEFINE:   -I %S/Inputs/mymod \
// DEFINE:   -Xfrontend -experimental-allow-module-with-compiler-errors \
// DEFINE:   -Xcc -D__INDEX_BUILD__=1 -D__INDEX_BUILD__ \
// DEFINE:   -Xcc -Xclang -Xcc -fallow-pcm-with-compiler-errors \
// DEFINE:   -Xcc -Xclang -Xcc -fmodule-format=raw \
// DEFINE:   -Xcc -Xclang -Xcc -detailed-preprocessing-record \
// DEFINE:   %s

// RUN: %swiftc_driver -emit-module -emit-module-path themod.swiftmodule -Xfrontend -experimental-skip-all-function-bodies  %{args}
// RUN: %swiftc_driver -index-file -index-file-path %s -index-store-path %t/idx -index-ignore-system-modules %{args}
// RUN: %sourcekitd-test -req=sema %s -- %{args}
// RUN: %find_files %t%{fs-sep}mcp 'mymod-*.pcm' | count 1

import mymod
Repository: apple/swift
File: stdlib/public/Cxx/CxxPair.swift (Swift)

Code before:
//===----------------------------------------------------------------------===//
//
// This source file is part of the Swift.org open source project
//
// Copyright (c) 2014 - 2023 Apple Inc. and the Swift project authors
// Licensed under Apache License v2.0 with Runtime Library Exception
//
// See https://swift.org/LICENSE.txt for license information
// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
//
//===----------------------------------------------------------------------===//

/// A C++ type that represents a pair of two values.
///
/// C++ standard library type `std::pair` conforms to this protocol.
public protocol CxxPair<First, Second> : ~Copyable {
  associatedtype First: ~Copyable
  associatedtype Second: ~Copyable

  init(first: consuming First, second: consuming Second) // memberwise init, synthesized by Swift

  var first: First { get set }
  var second: Second { get set }
}


Review comment (suggestion):
Interesting! I'd expected this to have a slightly different shape, but I am not that familiar with how we write these protocols. Specifically, I'd expected that we want to me `CxxPair` conditionally `Copyable` based on the copyability of its generic arguments. 

Something like:
```
public protocol CxxPair<First:  ~Copyable, Second:  ~Copyable> : ~Copyable { ... }

extension CxxPair: Copyable where First: Copyable, Second: Copyable {}
```

Code after:
//===----------------------------------------------------------------------===//
//
// This source file is part of the Swift.org open source project
//
// Copyright (c) 2014 - 2023 Apple Inc. and the Swift project authors
// Licensed under Apache License v2.0 with Runtime Library Exception
//
// See https://swift.org/LICENSE.txt for license information
// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
//
//===----------------------------------------------------------------------===//

/// A C++ type that represents a pair of two values.
///
/// C++ standard library type `std::pair` conforms to this protocol.
public protocol CxxPair<First, Second> : ~Copyable {
  associatedtype First : ~Copyable
  associatedtype Second : ~Copyable

  init(first: consuming First, second: consuming Second) // memberwise init, synthesized by Swift

  var first: First { get set }
  var second: Second { get set }
}
Repository: apple/swift
File: test/Interop/Cxx/stdlib/use-std-pair-typechecker.swift (Swift)

Code before:
// RUN: not %target-swift-frontend %s -typecheck -I %S/Inputs -cxx-interoperability-mode=default -diagnostic-style llvm 2>&1 | %FileCheck %s

import StdPair

let u = HasMethodThatReturnsUnsafePair()
u.getUnsafePair()
// CHECK: error: value of type 'HasMethodThatReturnsUnsafePair' has no member 'getUnsafePair'
// CHECK: note: C++ method 'getUnsafePair' may return an interior pointer

u.getIteratorPair()
// CHECK: error: value of type 'HasMethodThatReturnsUnsafePair' has no member 'getIteratorPair'
// CHECK: note: C++ method 'getIteratorPair' may return an interior pointer

func takeCopyable<T: Copyable>(_ _: T) {} 
func takeCopyablePair(_ _: any CxxPair) {} 
func takeNonCopyablePair(_ _: consuming any CxxPair & ~Copyable) {} 

let p1 = NonCopyableFirst(first: NonCopyable(field: 1), second: 2)
takeCopyable(p1)
// CHECK: error: global function 'takeCopyable' requires that 'NonCopyableFirst' {{.*}} conform to 'Copyable'
// CHECK: note: 'where T: Copyable' is implicit here

takeCopyablePair(p1)
// CHECK: error: argument type 'NonCopyableFirst' {{.*}} does not conform to expected type 'Copyable'

takeNonCopyablePair(p1)

Review comment (suggestion):
I wonder if it would be better to use `-verify` to test this. 

Code after:
// RUN: %target-typecheck-verify-swift -I %S/Inputs -cxx-interoperability-mode=default -suppress-notes

import StdPair

let u = HasMethodThatReturnsUnsafePair()
u.getUnsafePair() // expected-error {{value of type 'HasMethodThatReturnsUnsafePair' has no member 'getUnsafePair'}}

u.getIteratorPair() // expected-error {{value of type 'HasMethodThatReturnsUnsafePair' has no member 'getIteratorPair'}}

func takeCopyable<T: Copyable>(_ _: T) {} 
func takeCopyablePair(_ _: any CxxPair) {} 
func takeNonCopyablePair(_ _: consuming any CxxPair & ~Copyable) {} 

let p1 = NonCopyableFirst(first: NonCopyable(field: 1), second: 2)
takeCopyable(p1) // expected-error {{conform to 'Copyable'}}

takeCopyablePair(p1) // expected-error {{does not conform to expected type 'Copyable'}}

takeNonCopyablePair(p1)

let p2 = NonCopyableBoth(first: NonCopyable(field: 3), second: NonCopyable(field: 4))

takeCopyable(p2) // expected-error {{conform to 'Copyable'}}

takeCopyablePair(p2) // expected-error {{does not conform to expected type 'Copyable'}}