ronantakizawa/codereview-bench huggingface dataset:
Repository: appwrite/appwrite
File: src/Appwrite/Migration/Version/V23.php (PHP)

Code before:
            Database::addFilter(
                $name,
                fn () => null,
                fn () => []
            );
        }

        Console::info('Migrating databases');
        $this->migrateDatabases();
    }

    /**
     * Migrate Databases.
     *
     * @return void
     * @throws Exception|Throwable
     */
    private function migrateDatabases(): void
    {
        if ($this->project->getId() === 'console') {
            return;
        }

        // since required + default can't be used together
        // so first creating the attribute then bulk updating the attribute
        $this->dbForProject->createAttributes('databases', [new Document([
            '$id' => ID::custom('type'),
            'type' => Database::VAR_STRING,
            'size' => 128,
            'required' => true,
            'signed' => true,
            'array' => false,
            'filters' => [],
        ])]);
        $this->dbForProject->updateDocuments('databases', new Document(['type' => 'sql']));
    }

}


Review comment (suggestion):
We have a method like `createAttributeFromCollection` that you can use to pull this info directly from `projects.php` to ensure they're consistent

Code after:
                $name,
                fn () => null,
                fn () => []
            );
        }

        Console::info('Migrating databases');
        $this->migrateDatabases();
    }

    /**
     * Migrate Databases.
     *
     * @return void
     * @throws Exception|Throwable
     */
    private function migrateDatabases(): void
    {
        if ($this->project->getId() === 'console') {
            return;
        }

        // since required + default can't be used together
        // so first creating the attribute then bulk updating the attribute
        $this->createAttributeFromCollection($this->dbForProject, 'databases', 'type');
        $this->dbForProject->updateDocuments('databases', new Document(['type' => 'sql']));
    }

}
Repository: appwrite/appwrite
File: src/Appwrite/Platform/Modules/Storage/Http/Buckets/Get.php (PHP)

Code before:
            throw new Exception(Exception::STORAGE_BUCKET_NOT_FOUND);
        }

        $this->addBucketStorageSize($dbForProject, $bucket);

        $response->dynamic($bucket, Response::MODEL_BUCKET);
    }

    private function addBucketStorageSize(Database $dbForProject, Document $bucket): void
    {
        $metric = str_replace(
            '{bucketInternalId}',
            $bucket->getSequence(),
            METRIC_BUCKET_ID_FILES_STORAGE
        );

        /**
         * StatsUsage does this create an ID -
         *
         * `$time = null;`\
         * `$id = md5("{$time}_{$period}_{$key}");`
         *
         * but when $time is null it just makes the $id as md5('_inf_' . $key);
         *
         * Why do this though?\
         * Using `getDocument()` below to leverage cache!
         */
        $statsDocId = md5('_inf_' . $metric);

        $storageStats = Authorization::skip(
            fn () => $dbForProject->getDocument(
                'stats',
                $statsDocId,
                [Query::select(['value'])]
            )
        );

        /**
         * The value can be 0 if stats were not aggregated when this request was made!
         */
        $totalSize = $storageStats->isEmpty() ? 0 : $storageStats->getAttribute('value', 0);

        $bucket->setAttribute('totalSize', $totalSize);
    }
}


Review comment (bug):
the size calculated in dbForProject is from stats usage not stats resources and which we have seen to be incorrect in many account. I'm not sure we should be doing this? Slow but we have same metric in logsDB from statsResources which counts directly from database and is correct but counting there is slow.

I don't know if we want to wait for Clickhouse implementation for usage for this.

Code after:
    ): void {
        $bucket = $dbForProject->getDocument('buckets', $bucketId);

        if ($bucket->isEmpty()) {
            throw new Exception(Exception::STORAGE_BUCKET_NOT_FOUND);
        }

        $dbForLogs = call_user_func($getLogsDB, $project);
        $this->addBucketStorageSize($dbForLogs, $bucket);

        $response->dynamic($bucket, Response::MODEL_BUCKET);
    }

    /**
     * Adds the latest aggregated bucket storage size from logs DB stats.
     */
    private function addBucketStorageSize(Database $dbForLogs, Document $bucket): void
    {
        $metric = str_replace(
            '{bucketInternalId}',
            $bucket->getSequence(),
            METRIC_BUCKET_ID_FILES_STORAGE
        );

        $statsDocId = md5('_inf_' . $metric);
        $storageStats = Authorization::skip(
            fn () => $dbForLogs->getDocument(
                'stats',
                $statsDocId,
                [Query::select(['value'])]
            )
        );

        /**
         * The value can be 0 if stats were not aggregated when this request was made!
         */
        $totalSize = $storageStats->isEmpty() ? 0 : $storageStats->getAttribute('value', 0);

        $bucket->setAttribute('totalSize', $totalSize);
    }
}
Repository: aptos-labs/aptos-core
File: third_party/move/tools/move-resource-viewer/src/fat_type.rs (Rust)

Code before:
    pub layout: FatStructLayout,
}

#[derive(Debug, Clone, Serialize, Deserialize, PartialOrd, Ord, PartialEq, Eq)]
pub(crate) enum FatStructLayout {
    Singleton(Vec<FatType>),
    Variants(Vec<Vec<FatType>>),
}

#[derive(Debug, Clone, Serialize, Deserialize, PartialOrd, Ord, PartialEq, Eq)]
pub(crate) struct FatFunctionType {
    pub args: Vec<FatType>,
    pub results: Vec<FatType>,
    pub abilities: AbilitySet,
}

#[derive(Debug, Clone, Serialize, Deserialize, PartialOrd, Ord, PartialEq, Eq)]
pub(crate) enum FatType {
    Bool,
    U8,
    U64,
    U128,
    Address,
    Signer,
    Vector(Box<FatType>),
    Struct(FatStructRef),
    Reference(Box<FatType>),
    MutableReference(Box<FatType>),
    TyParam(usize),
    // NOTE: Added in bytecode version v6, do not reorder!
    U16,
    U32,
    U256,
    // NOTE: Added in bytecode version v8, do not reorder!
    Function(Box<FatFunctionType>),
    // `Runtime` and `RuntimeVariants` are used for typing
    // captured structures in closures, for which we only know
    // the raw layout (no struct name, no field names).
    Runtime(Vec<FatType>),
    RuntimeVariants(Vec<Vec<FatType>>),
    // NOTE: Added in bytecode version v9, do not reorder!
    I8,
    I16,
    I32,
    I64,
    I128,
    I256,
}

/// A representation for fat structs which assumes
/// that they are interned, enabling fast pointer comparison.

Review comment (suggestion):
We can probably make vectors pointers too? To reduce clones, e.g., currently we construct a full `FatType` and then call `subst` on it which just clones these

Code after:
    pub layout: FatStructLayout,
}

#[derive(Debug, Clone, Serialize, Deserialize, PartialOrd, Ord, PartialEq, Eq)]
pub(crate) enum FatStructLayout {
    Singleton(Vec<FatType>),
    Variants(Vec<Vec<FatType>>),
}

#[derive(Debug, Clone, Serialize, Deserialize, PartialOrd, Ord, PartialEq, Eq)]
pub(crate) struct FatFunctionType {
    pub args: Vec<FatType>,
    pub results: Vec<FatType>,
    pub abilities: AbilitySet,
}

// INVARIANT: this type need to stay crate local. See discussion at `FatStructRef`.
#[derive(Debug, Clone, Serialize, Deserialize, PartialOrd, Ord, PartialEq, Eq)]
pub(crate) enum FatType {
    Bool,
    U8,
    U64,
    U128,
    Address,
    Signer,
    Vector(Box<FatType>),
    Struct(FatStructRef),
    Reference(Box<FatType>),
    MutableReference(Box<FatType>),
    TyParam(usize),
    // NOTE: Added in bytecode version v6, do not reorder!
    U16,
    U32,
    U256,
    // NOTE: Added in bytecode version v8, do not reorder!
    Function(Box<FatFunctionType>),
    // `Runtime` and `RuntimeVariants` are used for typing
    // captured structures in closures, for which we only know
    // the raw layout (no struct name, no field names).
    Runtime(Vec<FatType>),
    RuntimeVariants(Vec<Vec<FatType>>),
    // NOTE: Added in bytecode version v9, do not reorder!
    I8,
    I16,
    I32,
    I64,
    I128,
    I256,
}

/// A representation for fat structs which assumes
Repository: aptos-labs/aptos-core
File: aptos-move/e2e-move-tests/src/tests/function_caches.rs (Rust)

Code before:
fn test_function_caches_for_recursive_functions_do_not_leak_memory() {
    let mut h = MoveHarness::new();
    let acc = h.new_account_at(AccountAddress::from_hex_literal("0x99").unwrap());

    let status = publish(
        &mut h,
        &acc,
        r#"
        module 0x99::m {
            public entry fun factorial(n: u64) {
                factorial_impl(n);
            }

            fun factorial_impl(n: u64): u64 {
                if (n <= 1) {
                    1
                } else {
                    n * factorial_impl(n - 1)
                }
            }
        }
        "#,
    );
    assert_success!(status);

    let run_factorial = || {
        for _ in 0..300 {
            let status = h.run_entry_function(
                &acc,
                str::parse("0x99::m::factorial").unwrap(),
                vec![],
                vec![bcs::to_bytes(&4_u64).unwrap()],
            );
            assert_success!(status);
        }
    };

    let measure_memory_growth = || {
        let start = memory_stats::memory_stats().unwrap().physical_mem;
        run_factorial();
        memory_stats::memory_stats()
            .unwrap()
            .physical_mem
            .saturating_sub(start)
    };

    // Warmup.
    run_factorial();

    // Memory growth later in time should be smaller than at the beginning.
    let a = measure_memory_growth();

Review comment (suggestion):
I think you need `mut` here to make it compile because it uses `h` mutably:


```suggestion
    let mut run_factorial = || {
```

Code after:
fn test_function_caches_for_recursive_functions_do_not_leak_memory() {
    let mut h = MoveHarness::new();
    let acc = h.new_account_at(AccountAddress::from_hex_literal("0x99").unwrap());

    let status = publish(
        &mut h,
        &acc,
        r#"
        module 0x99::m {
            public entry fun factorial(n: u64) {
                factorial_impl(n);
            }

            fun factorial_impl(n: u64): u64 {
                if (n <= 1) {
                    1
                } else {
                    n * factorial_impl(n - 1)
                }
            }
        }
        "#,
    );
    assert_success!(status);

    // Warmup.
    run_factorial(&mut h, &acc);

    // Memory growth later in time should be smaller than at the beginning.
    let a = run_factorial_measure_memory_growth(&mut h, &acc);
    let b = run_factorial_measure_memory_growth(&mut h, &acc);
    assert!(b <= a);
}

fn run_factorial(h: &mut MoveHarness, acc: &Account) {
    for _ in 0..300 {
        let status = h.run_entry_function(
            acc,
            str::parse("0x99::m::factorial").unwrap(),
            vec![],
            vec![bcs::to_bytes(&4_u64).unwrap()],
        );
        assert_success!(status);
    }
}

fn run_factorial_measure_memory_growth(h: &mut MoveHarness, acc: &Account) -> usize {
    let start = memory_stats::memory_stats().unwrap().virtual_mem;
    run_factorial(h, acc);
    memory_stats::memory_stats()
        .unwrap()
Repository: aptos-labs/aptos-core
File: third_party/move/move-vm/runtime/src/interpreter.rs (Rust)

Code before:

            // If out of the loop - it was a branch.
            if self.pc as usize >= code.len() {
                return Err(PartialVMError::new(StatusCode::PC_OVERFLOW));
            }
        }
    }

    pub(crate) fn location(&self) -> Location {
        match self.function.module_id() {
            None => Location::Script,
            Some(id) => Location::Module(id.clone()),
        }
    }

    #[cold]
    fn stack_size_mismatch_error(&self, expected: usize, actual: usize) -> PartialVMError {
        stack_size_mismatch_error_for_function(&self.function, expected, actual)
    }
}

#[cold]
fn stack_size_mismatch_error_for_function(
    function: &LoadedFunction,
    expected: usize,
    actual: usize,
) -> PartialVMError {
    let err = PartialVMError::new_invariant_violation(format!(
        "Stack size mismatch when returning from {}: expected: {}, got: {}",
        function.name_as_pretty_string(),
        expected,
        actual
    ));
    err.with_sub_status(EPARANOID_FAILURE)
}


Review comment (suggestion):
nit: I think swapping actual and expected would read a bit more naturally.

```suggestion
    actual: usize,
    expected: usize,
```

Code after:

            // If out of the loop - it was a branch.
            if self.pc as usize >= code.len() {
                return Err(PartialVMError::new(StatusCode::PC_OVERFLOW));
            }
        }
    }

    pub(crate) fn location(&self) -> Location {
        match self.function.module_id() {
            None => Location::Script,
            Some(id) => Location::Module(id.clone()),
        }
    }

    #[cold]
    fn stack_size_mismatch_error(&self, actual: usize, expected: usize) -> PartialVMError {
        stack_size_mismatch_error_for_function(&self.function, actual, expected)
    }
}

#[cold]
fn stack_size_mismatch_error_for_function(
    function: &LoadedFunction,
    actual: usize,
    expected: usize,
) -> PartialVMError {
    let err = PartialVMError::new_invariant_violation(format!(
        "Stack size mismatch when returning from {}: expected: {}, got: {}",
        function.name_as_pretty_string(),
        expected,
        actual
    ));
    err.with_sub_status(EPARANOID_FAILURE)
}
Repository: astral-sh/ruff
File: crates/ty_server/tests/e2e/inlay_hints.rs (Rust)

Code before:
foo(1)
";

    let mut server = TestServerBuilder::new()?
        .with_initialization_options(ClientOptions::default())
        .with_workspace(workspace_root, None)?
        .with_file(foo, foo_content)?
        .enable_inlay_hints(true)
        .build()?
        .wait_until_workspaces_are_initialized()?;

    server.open_text_document(foo, &foo_content, 1);
    let _ = server.await_notification::<PublishDiagnostics>()?;

    let hints = server
        .inlay_hints_request(foo, Range::new(Position::new(0, 0), Position::new(6, 0)))?
        .unwrap();

    insta::assert_json_snapshot!(hints, @r#"
    [
      {
        "position": {
          "line": 0,
          "character": 1
        },
        "label": [
          {
            "value": ": Literal[1]"
          }
        ],
        "kind": 1
      },
      {
        "position": {
          "line": 5,
          "character": 4
        },
        "label": [
          {
            "value": "a="
          }
        ],
        "kind": 2
      }
    ]
    "#);

    Ok(())
}

/// Tests that disabling variable types inlay hints works correctly.

Review comment (suggestion):
These change because we now use `InlayHintLabel::LabelParts` instead of `InlayHintLabel::String`

Code after:
foo(1)
";

    let mut server = TestServerBuilder::new()?
        .with_initialization_options(ClientOptions::default())
        .with_workspace(workspace_root, None)?
        .with_file(foo, foo_content)?
        .enable_inlay_hints(true)
        .build()?
        .wait_until_workspaces_are_initialized()?;

    server.open_text_document(foo, &foo_content, 1);
    let _ = server.await_notification::<PublishDiagnostics>()?;

    let hints = server
        .inlay_hints_request(foo, Range::new(Position::new(0, 0), Position::new(6, 0)))?
        .unwrap();

    insta::assert_json_snapshot!(hints, @r#"
    [
      {
        "position": {
          "line": 0,
          "character": 1
        },
        "label": [
          {
            "value": ": "
          },
          {
            "value": "Literal[1]"
          }
        ],
        "kind": 1
      },
      {
        "position": {
          "line": 5,
          "character": 4
        },
        "label": [
          {
            "value": "a"
          },
          {
            "value": "="
          }
        ],
        "kind": 2
      }
    ]
Repository: astral-sh/ruff
File: crates/ty_ide/src/inlay_hints.rs (Rust)

Code before:
use std::fmt;

use crate::Db;
use ruff_db::files::File;
use ruff_db::parsed::parsed_module;
use ruff_python_ast::visitor::source_order::{self, SourceOrderVisitor, TraversalSignal};
use ruff_python_ast::{AnyNodeRef, Expr, Stmt};
use ruff_text_size::{Ranged, TextRange, TextSize};
use ty_python_semantic::types::{Type, inlay_hint_function_argument_details};
use ty_python_semantic::{HasType, SemanticModel};

#[derive(Debug, Clone)]
pub struct InlayHint {
    pub position: TextSize,
    pub kind: InlayHintKind,
    pub label: InlayHintLabel,
}

impl InlayHint {
    pub fn type_hint(position: TextSize, ty: Type, db: &dyn Db) -> Self {
        let mut label = InlayHintLabel::simple(ty.display(db).to_string(), None);

        label.prepend_str(": ");

        Self {
            position,
            kind: InlayHintKind::Type,
            label,
        }
    }

    pub fn call_argument_name(position: TextSize, name: &str) -> Self {
        let mut label = InlayHintLabel::simple(name.to_string(), None);

        label.append_str("=");

        Self {
            position,
            kind: InlayHintKind::CallArgumentName,
            label,
        }
    }

    pub fn display(&self) -> InlayHintDisplay<'_> {
        InlayHintDisplay { inlay_hint: self }

Review comment (suggestion):
I think it'd be useful to add basic documentation for all the `pub` items in this module.

Code after:
use std::{fmt, vec};

use crate::Db;
use ruff_db::files::File;
use ruff_db::parsed::parsed_module;
use ruff_python_ast::visitor::source_order::{self, SourceOrderVisitor, TraversalSignal};
use ruff_python_ast::{AnyNodeRef, Expr, Stmt};
use ruff_text_size::{Ranged, TextRange, TextSize};
use ty_python_semantic::types::{Type, inlay_hint_function_argument_details};
use ty_python_semantic::{HasType, SemanticModel};

#[derive(Debug, Clone)]
pub struct InlayHint {
    pub position: TextSize,
    pub kind: InlayHintKind,
    pub label: InlayHintLabel,
}

impl InlayHint {
    fn variable_type(position: TextSize, ty: Type, db: &dyn Db) -> Self {
        let label_parts = vec![
            ": ".into(),
            InlayHintLabelPart::new(ty.display(db).to_string()),
        ];

        Self {
            position,
            kind: InlayHintKind::Type,
            label: InlayHintLabel { parts: label_parts },
        }
    }

    fn call_argument_name(position: TextSize, name: &str) -> Self {
        let label_parts = vec![InlayHintLabelPart::new(name), "=".into()];

        Self {
            position,
            kind: InlayHintKind::CallArgumentName,
            label: InlayHintLabel { parts: label_parts },
        }
    }

    pub fn display(&self) -> InlayHintDisplay<'_> {
        InlayHintDisplay { inlay_hint: self }
    }
Repository: astral-sh/ruff
File: crates/ty_ide/src/inlay_hints.rs (Rust)

Code before:

        Self {
            position,
            kind: InlayHintKind::CallArgumentName,
            label,
        }
    }

    pub fn display(&self) -> InlayHintDisplay<'_> {
        InlayHintDisplay { inlay_hint: self }
    }
}

#[derive(Debug, Clone)]
pub enum InlayHintKind {
    Type,
    CallArgumentName,
}

#[derive(Debug, Clone)]
pub struct InlayHintLabel {
    pub parts: Vec<InlayHintLabelPart>,
}

impl InlayHintLabel {
    pub fn simple(s: impl Into<String>, target: Option<crate::NavigationTarget>) -> InlayHintLabel {
        InlayHintLabel {
            parts: vec![InlayHintLabelPart {
                text: s.into(),
                target,
            }],
        }
    }

    pub fn prepend_str(&mut self, s: &str) {
        match &mut *self.parts {
            [InlayHintLabelPart { text, target: None }, ..] => text.insert_str(0, s),
            _ => self.parts.insert(
                0,
                InlayHintLabelPart {
                    text: s.into(),
                    target: None,
                },
            ),
        }
    }

    pub fn append_str(&mut self, s: &str) {
        match &mut *self.parts {
            [.., InlayHintLabelPart { text, target: None }] => text.push_str(s),
            _ => self.parts.push(InlayHintLabelPart {

Review comment (suggestion):
I think `simple` sounds confusing, what does simple mean in the context of `InlayHintLabel`? Based on my other suggestion to introduce `with_target` for `InlayHintLabelPart`, we could rename this to `single(part: InlayHintLabelPart)` and let the caller construct the `InlayHintLabelPart` or just a `new(parts: Vec<InlayHintLabelPart>`) as `InlayHintLabelPart` is a public type.

Code after:
        Self {
            position,
            kind: InlayHintKind::CallArgumentName,
            label: InlayHintLabel { parts: label_parts },
        }
    }

    pub fn display(&self) -> InlayHintDisplay<'_> {
        InlayHintDisplay { inlay_hint: self }
    }
}

#[derive(Debug, Clone)]
pub enum InlayHintKind {
    Type,
    CallArgumentName,
}

#[derive(Debug, Clone)]
pub struct InlayHintLabel {
    parts: Vec<InlayHintLabelPart>,
}

impl InlayHintLabel {
    pub fn parts(&self) -> &[InlayHintLabelPart] {
        &self.parts
    }
}

pub struct InlayHintDisplay<'a> {
    inlay_hint: &'a InlayHint,
}

impl fmt::Display for InlayHintDisplay<'_> {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> std::fmt::Result {
        for part in &self.inlay_hint.label.parts {
            write!(f, "{}", part.text)?;
        }
        Ok(())
    }
}

#[derive(Default, Debug, Clone)]
pub struct InlayHintLabelPart {
    text: String,

    target: Option<crate::NavigationTarget>,
}

impl InlayHintLabelPart {
    pub fn new(text: impl Into<String>) -> Self {