ronantakizawa/codereview-bench huggingface dataset:
Repository: sxyazi/yazi
File: yazi-plugin/src/utils/log.rs (Rust)

Code before:
use mlua::{Function, Lua, MultiValue};
use tracing::{debug, error};

use super::Utils;

impl Utils {
	pub(super) fn dbg(lua: &Lua) -> mlua::Result<Function> {
		lua.create_function(|_, values: MultiValue| {
			let s = values.into_iter().map(|v| format!("{v:#?}")).collect::<Vec<_>>().join(" ");
			Ok(debug!("{s}"))
		})
	}

	pub(super) fn err(lua: &Lua) -> mlua::Result<Function> {
		lua.create_function(|_, values: MultiValue| {
			let s = values.into_iter().map(|v| format!("{v:#?}")).collect::<Vec<_>>().join(" ");
			Ok(error!("{s}"))
		})
	}
}


Review comment (bug):
`format_one` can now fail (e.g., `ud.borrow::<...>()` errors on Lua userdata borrow conflicts), which would make `ya.dbg()` / `ya.err()` throw and potentially break plugins. Logging helpers should be best-effort; consider catching borrow/format errors and falling back to `Debug` formatting instead of propagating the error.

Code after:
		for value in values {
			if !s.is_empty() {
				s.push(' ');
			}
			Utils::format_one(&mut s, value)?;
		}
		Ok(s)
	}

	fn format_one(buf: &mut String, value: Value) -> anyhow::Result<()> {
		let Value::UserData(ud) = &value else {
			return Ok(write!(buf, "{value:#?}")?);
		};

		let id = ud.type_id();
		let ptr = ud.to_pointer();
		Ok(match id {
			Some(t) if t == TypeId::of::<yazi_binding::Url>() => {
				write!(buf, "Url({ptr:?}): {:?}", **ud.borrow::<yazi_binding::Url>()?)?
			}
			Some(t) if t == TypeId::of::<yazi_binding::Path>() => {
				write!(buf, "Path({ptr:?}): {:?}", **ud.borrow::<yazi_binding::Path>()?)?
			}
			Some(t) if t == TypeId::of::<yazi_binding::Id>() => {
				write!(buf, "Id({ptr:?}): {}", **ud.borrow::<yazi_binding::Id>()?)?
			}
			_ => write!(buf, "{value:#?}")?,
		})
	}
}
Repository: sxyazi/yazi
File: yazi-shared/src/sync_cell.rs (Rust)

Code before:
use std::{cell::Cell, fmt::{Debug, Display, Formatter}, ops::Deref};

use serde::{Deserialize, Deserializer, Serialize, Serializer};

/// [`Cell`], but [`Sync`].
///
/// This is just an `Cell`, except it implements `Sync`
/// if `T` implements `Sync`.
pub struct SyncCell<T: ?Sized>(Cell<T>);

unsafe impl<T: ?Sized + Sync> Sync for SyncCell<T> {}

impl<T> SyncCell<T> {
	#[inline]
	pub const fn new(value: T) -> Self { Self(Cell::new(value)) }
}

impl<T: Default> Default for SyncCell<T> {
	fn default() -> Self { Self::new(T::default()) }
}

impl<T> Deref for SyncCell<T> {
	type Target = Cell<T>;

	#[inline]
	fn deref(&self) -> &Self::Target { &self.0 }
}

impl<T: Copy> Clone for SyncCell<T> {
	#[inline]
	fn clone(&self) -> Self { Self::new(self.get()) }
}

Review comment (suggestion):
The doc comment grammar reads awkwardly: “This is just an `Cell`”. Since this is user-facing documentation, it should be “a `Cell`”.
```suggestion
/// This is just a `Cell`, except it implements `Sync`
```

Code after:
use std::{cell::Cell, fmt::{Debug, Display, Formatter}, ops::Deref};

use serde::{Deserialize, Deserializer, Serialize, Serializer};

/// [`Cell`], but [`Sync`].
///
/// This is just a `Cell`, except it implements `Sync`
/// if `T` implements `Sync`.
pub struct SyncCell<T: ?Sized>(Cell<T>);

unsafe impl<T: ?Sized + Sync> Sync for SyncCell<T> {}

impl<T> SyncCell<T> {
	#[inline]
	pub const fn new(value: T) -> Self { Self(Cell::new(value)) }
}

impl<T: Default> Default for SyncCell<T> {
	fn default() -> Self { Self::new(T::default()) }
}

impl<T> Deref for SyncCell<T> {
	type Target = Cell<T>;

	#[inline]
	fn deref(&self) -> &Self::Target { &self.0 }
}

impl<T: Copy> Clone for SyncCell<T> {
	#[inline]
	fn clone(&self) -> Self { Self::new(self.get()) }
}
Repository: sxyazi/yazi
File: yazi-shared/src/last_value.rs (Rust)

Code before:
use std::sync::Arc;

use parking_lot::Mutex;
use tokio::sync::Notify;

#[derive(Clone, Debug, Default)]
pub struct LastValue<T> {
	inner: Arc<(Notify, Mutex<Option<T>>)>,
}

impl<T> LastValue<T> {
	pub fn set(&self, data: T) {
		*self.inner.1.lock() = Some(data);
		self.inner.0.notify_waiters();
	}

	pub async fn get(&self) -> T {
		loop {
			if let Some(data) = self.inner.1.lock().take() {
				return data;
			}

			self.inner.0.notified().await;
		}
	}
}


Review comment (suggestion):
`LastValue::get` has a race that can miss notifications: if `set()` runs after the `lock().take()` check but before `notified().await` is created/polled, the notify is lost and `get()` can await forever even though the value is already `Some(...)` in the mutex. Use the standard `Notify` pattern where you create the `notified()` future *before* checking the shared state (or use `notify.notified()` + re-check loop), so notifications between the check and await are not lost.
```suggestion
			// Create the notification future before checking the shared state
			let notified = self.inner.0.notified();

			if let Some(data) = self.inner.1.lock().take() {
				return data;
			}

			// Await the notification and then re-check in the next loop iteration
			notified.await;
```

Code after:
use std::sync::Arc;

use parking_lot::Mutex;
use tokio::sync::Notify;

#[derive(Clone, Debug, Default)]
pub struct LastValue<T> {
	inner: Arc<(Notify, Mutex<Option<T>>)>,
}

impl<T> LastValue<T> {
	pub fn set(&self, data: T) {
		*self.inner.1.lock() = Some(data);
		self.inner.0.notify_waiters();
	}

	pub async fn get(&self) -> T {
		loop {
			let notified = self.inner.0.notified();
			if let Some(data) = self.inner.1.lock().take() {
				return data;
			}

			notified.await;
		}
	}
}
Repository: sxyazi/yazi
File: yazi-actor/src/app/accept_payload.rs (Rust)

Code before:
use yazi_plugin::LUA;
use yazi_shared::data::Data;

use crate::{Actor, Ctx};

pub struct AcceptPayload;

impl Actor for AcceptPayload {
	type Options = Payload<'static>;

	const NAME: &str = "accept_payload";

	fn act(cx: &mut Ctx, payload: Payload) -> Result<Data> {
		let kind = payload.body.kind();
		let lock = if payload.receiver == 0 || payload.receiver != payload.sender {
			REMOTE.read()
		} else {
			LOCAL.read()
		};

		let Some(handlers) = lock.get(kind).filter(|&m| !m.is_empty()).cloned() else {
			succ!();
		};
		if handlers.is_empty() {
			succ!();
		}
		drop(lock);

		let kind = kind.to_owned();
		succ!(Lives::scope(&cx.core, || {
			let body = payload.body.into_lua(&LUA)?;
			for (id, cb) in handlers {
				if let Err(e) = runtime_scope!(LUA, &id, cb.call::<()>(body.clone())) {
					error!("Failed to run `{kind}` event handler in your `{id}` plugin: {e}");
				}
			}
			Ok(())
		})?);
	}
}


Review comment (refactor):
The `handlers.is_empty()` check is redundant here because `lock.get(kind).filter(|&m| !m.is_empty())` already guarantees `handlers` is non-empty after `cloned()`. Removing the extra check would simplify the control flow.
```suggestion

```

Code after:
use yazi_plugin::LUA;
use yazi_shared::data::Data;

use crate::{Actor, Ctx};

pub struct AcceptPayload;

impl Actor for AcceptPayload {
	type Options = Payload<'static>;

	const NAME: &str = "accept_payload";

	fn act(cx: &mut Ctx, payload: Payload) -> Result<Data> {
		let kind = payload.body.kind();
		let lock = if payload.receiver == 0 || payload.receiver != payload.sender {
			REMOTE.read()
		} else {
			LOCAL.read()
		};

		let Some(handlers) = lock.get(kind).filter(|&m| !m.is_empty()).cloned() else { succ!() };
		drop(lock);

		let kind = kind.to_owned();
		succ!(Lives::scope(&cx.core, || {
			let body = payload.body.into_lua(&LUA)?;
			for (id, cb) in handlers {
				if let Err(e) = runtime_scope!(LUA, &id, cb.call::<()>(body.clone())) {
					error!("Failed to run `{kind}` event handler in your `{id}` plugin: {e}");
				}
			}
			Ok(())
		})?);
	}
}
Repository: sxyazi/yazi
File: yazi-config/src/lib.rs (Rust)

Code before:
	let stdout = &mut *TTY.lockout();

	writeln!(stdout, "{e}")?;
	if let Some(src) = e.source() {
		writeln!(stdout, "\nCaused by:\n{src}")?;
	}

	use crossterm::style::{Attribute, Print, SetAttributes};
	crossterm::execute!(
		stdout,
		SetAttributes(Attribute::Reverse.into()),
		SetAttributes(Attribute::Bold.into()),
		Print("Press <Enter> to continue with preset settings..."),
		SetAttributes(Attribute::Reset.into()),
		Print("\n"),
	)?;

	TTY.reader().read_exact(&mut [0])?;
	Ok(())
}

pub(crate) fn parse_recoverable<'a>(
	input: &'a str,
) -> Result<Spanned<DeTable<'a>>, toml::de::Error> {
	let (table, errors) = DeTable::parse_recoverable(input);
	if let Some(err) = errors.into_iter().next() { Err(err) } else { Ok(table) }
}

pub(crate) fn error_with_input<T>(
	result: Result<T, toml::de::Error>,
	input: &str,
) -> Result<T, toml::de::Error> {
	result.map_err(|mut err| {
		err.set_input(Some(input));
		err
	})
}


Review comment (bug):
`parse_recoverable` returns the first parse error directly without calling `set_input(Some(input))`. Since later deserialization errors are explicitly augmented with the input via `error_with_input`, consider doing the same here so parse failures also retain source context when formatted.
```suggestion
	if let Some(mut err) = errors.into_iter().next() {
		err.set_input(Some(input));
		Err(err)
	} else {
		Ok(table)
	}
```

Code after:

	writeln!(stdout, "{e}")?;
	if let Some(src) = e.source() {
		writeln!(stdout, "\nCaused by:\n{src}")?;
	}

	use crossterm::style::{Attribute, Print, SetAttributes};
	crossterm::execute!(
		stdout,
		SetAttributes(Attribute::Reverse.into()),
		SetAttributes(Attribute::Bold.into()),
		Print("Press <Enter> to continue with preset settings..."),
		SetAttributes(Attribute::Reset.into()),
		Print("\n"),
	)?;

	TTY.reader().read_exact(&mut [0])?;
	Ok(())
}

pub(crate) fn error_with_input<T>(
	result: Result<T, toml::de::Error>,
	input: &str,
) -> Result<T, toml::de::Error> {
	result.map_err(|mut err| {
		err.set_input(Some(input));
		err
	})
}
Repository: symfony/symfony
File: src/Symfony/Component/Workflow/Event/Event.php (PHP)

Code before:
 *
 * For the full copyright and license information, please view the LICENSE
 * file that was distributed with this source code.
 */

namespace Symfony\Component\Workflow\Event;

use Symfony\Component\Workflow\Marking;
use Symfony\Component\Workflow\Transition;
use Symfony\Component\Workflow\WorkflowInterface;
use Symfony\Contracts\EventDispatcher\Event as BaseEvent;

/**
 * @author Fabien Potencier <fabien@symfony.com>
 * @author Grégoire Pineau <lyrixx@lyrixx.info>
 * @author Carlos Pereira De Amorim <carlos@shauri.fr>
 */
class Event extends BaseEvent
{
    private $subject;
    private $marking;
    private $transition;
    private $workflow;
    private $context;

    public function __construct(object $subject, Marking $marking, Transition $transition = null, WorkflowInterface $workflow = null, ?array $context = null)
    {
        $this->subject = $subject;
        $this->marking = $marking;
        $this->transition = $transition;
        $this->workflow = $workflow;
        $this->context = $context;
    }

    public function getMarking()
    {
        return $this->marking;
    }

    public function getSubject()
    {
        return $this->subject;
    }

    public function getTransition()
    {
        return $this->transition;
    }

    public function getWorkflow(): WorkflowInterface
    {

Review comment (suggestion):
is context nullability desired? an empty array would work also isnt it, giving us a consistent return type for getContext()

Code after:
 *
 * For the full copyright and license information, please view the LICENSE
 * file that was distributed with this source code.
 */

namespace Symfony\Component\Workflow\Event;

use Symfony\Component\Workflow\Marking;
use Symfony\Component\Workflow\Transition;
use Symfony\Component\Workflow\WorkflowInterface;
use Symfony\Contracts\EventDispatcher\Event as BaseEvent;

/**
 * @author Fabien Potencier <fabien@symfony.com>
 * @author Grégoire Pineau <lyrixx@lyrixx.info>
 * @author Carlos Pereira De Amorim <carlos@shauri.fr>
 */
class Event extends BaseEvent
{
    protected $context;
    private $subject;
    private $marking;
    private $transition;
    private $workflow;

    public function __construct(object $subject, Marking $marking, Transition $transition = null, WorkflowInterface $workflow = null, array $context = [])
    {
        $this->subject = $subject;
        $this->marking = $marking;
        $this->transition = $transition;
        $this->workflow = $workflow;
        $this->context = $context;
    }

    public function getMarking()
    {
        return $this->marking;
    }

    public function getSubject()
    {
        return $this->subject;
    }

    public function getTransition()
    {
        return $this->transition;
    }

    public function getWorkflow(): WorkflowInterface
    {
Repository: symfony/symfony
File: src/Symfony/Component/Translation/Translator.php (PHP)

Code before:
        $this->assertValidLocale($locale);
        $this->locale = $locale;
    }

    /**
     * {@inheritdoc}
     */
    public function getLocale(): string
    {
        return $this->locale ?: (class_exists(\Locale::class) ? \Locale::getDefault() : 'en');
    }

    /**
     * Sets the fallback locales.
     *
     * @param string[] $locales
     *
     * @throws InvalidArgumentException If a locale contains invalid characters
     */
    public function setFallbackLocales(array $locales)
    {
        // needed as the fallback locales are linked to the already loaded catalogues
        $this->catalogues = [];

        foreach ($locales as $locale) {
            $this->assertValidLocale($locale);
        }

        $this->fallbackLocaleProvider->setFallbackLocales($locales);
        $this->cacheVary['fallback_locales'] = $locales;
    }

    /**
     * @internal
     */
    public function getFallbackLocales(): array
    {
        return $this->fallbackLocaleProvider->getFallbackLocales();
    }

    /**
     * {@inheritdoc}
     */
    public function trans(?string $id, array $parameters = [], string $domain = null, string $locale = null): string
    {
        if (null === $id || '' === $id) {
            return '';
        }

        if (null === $domain) {
            $domain = 'messages';

Review comment (suggestion):
It would be nice if the `FallbackLocaleProvider` could use this guard itself, but I am unsure where to move the code.

Code after:
        $this->locale = $locale;
    }

    public function getLocale(): string
    {
        return $this->locale ?: (class_exists(\Locale::class) ? \Locale::getDefault() : 'en');
    }

    /**
     * Sets the fallback locales.
     *
     * @param string[] $locales
     *
     * @throws InvalidArgumentException If a locale contains invalid characters
     */
    public function setFallbackLocales(array $locales): void
    {
        if ($this->fallbackLocales === $locales) {
            return;
        }

        $this->localeFallbackProvider = new LocaleFallbackProvider($locales);
        $this->fallbackLocales = $this->cacheVary['fallback_locales'] = $locales;
        $this->catalogues = [];
    }

    /**
     * @internal
     */
    public function getFallbackLocales(): array
    {
        return $this->fallbackLocales;
    }

    public function addGlobalParameter(string $id, string|int|float|TranslatableInterface $value): void
    {
        $this->globalParameters[$id] = $value;
        $this->globalTranslatedParameters = [];
    }

    public function getGlobalParameters(): array
    {
        return $this->globalParameters;
    }

    public function trans(?string $id, array $parameters = [], ?string $domain = null, ?string $locale = null): string
    {
        if (null === $id || '' === $id) {
            return '';
        }
Repository: symfony/symfony
File: src/Symfony/Component/CssSelector/Node/RelationNode.php (PHP)

Code before:
 * @internal
 */
class RelationNode extends AbstractNode
{
    private NodeInterface $selector;
    private array $arguments;

    public function __construct(NodeInterface $selector, array $arguments)
    {
        $this->selector = $selector;
        $this->arguments = $arguments;
    }

    public function getSelector(): NodeInterface
    {
        return $this->selector;
    }

    public function getArguments(): array
    {
        return $this->arguments;
    }

    public function getSpecificity(): Specificity
    {
        return $this->selector->getSpecificity()->plus(new Specificity(0, 1, 0));
    }

    public function __toString(): string
    {
        $arguments = implode(', ', array_map(fn (Token $token) => "'".$token->getValue()."'", $this->arguments));

        return sprintf('%s[%s:has(%s)]', $this->getNodeName(), $this->selector, $arguments ? '['.$arguments.']' : '');
    }
}


Review comment (suggestion):
the specificity of `:has()` is not `(0, 1, 0)` but the specificity of the most specific selector in its arguments

Code after:
class RelationNode extends AbstractNode
{
    public function __construct(
        private NodeInterface $selector,
        private string $combinator,
        private NodeInterface $subSelector,
    ) {
    }

    public function getSelector(): NodeInterface
    {
        return $this->selector;
    }

    public function getCombinator(): string
    {
        return $this->combinator;
    }

    public function getSubSelector(): NodeInterface
    {
        return $this->subSelector;
    }

    public function getSpecificity(): Specificity
    {
        return $this->selector->getSpecificity()->plus($this->subSelector->getSpecificity());
    }

    public function __toString(): string
    {
        return \sprintf('%s[%s:has(%s)]', $this->getNodeName(), $this->selector, $this->subSelector);
    }
}
Repository: symfony/symfony
File: src/Symfony/Component/CssSelector/Node/RelationNode.php (PHP)

Code before:
 * (c) Fabien Potencier <fabien@symfony.com>
 *
 * For the full copyright and license information, please view the LICENSE
 * file that was distributed with this source code.
 */

namespace Symfony\Component\CssSelector\Node;

use Symfony\Component\CssSelector\Parser\Token;

/**
 * Represents a "<selector>:has(<arguments>)" node.
 *
 * This component is a port of the Python cssselect library,
 * which is copyright Ian Bicking, @see https://github.com/SimonSapin/cssselect.
 *
 * @author Jean-François Simon <jeanfrancois.simon@sensiolabs.com>
 *
 * @internal
 */
class RelationNode extends AbstractNode
{
    private NodeInterface $selector;
    private array $arguments;

    public function __construct(NodeInterface $selector, array $arguments)
    {
        $this->selector = $selector;
        $this->arguments = $arguments;
    }

    public function getSelector(): NodeInterface
    {
        return $this->selector;
    }

    public function getArguments(): array
    {
        return $this->arguments;
    }

    public function getSpecificity(): Specificity
    {
        return $this->selector->getSpecificity()->plus(new Specificity(0, 1, 0));
    }

    public function __toString(): string
    {
        $arguments = implode(', ', array_map(fn (Token $token) => "'".$token->getValue()."'", $this->arguments));

        return sprintf('%s[%s:has(%s)]', $this->getNodeName(), $this->selector, $arguments ? '['.$arguments.']' : '');

Review comment (suggestion):
you should document the type for `$arguments` as `list<NodeInterface>`

Code after:
 * (c) Fabien Potencier <fabien@symfony.com>
 *
 * For the full copyright and license information, please view the LICENSE
 * file that was distributed with this source code.
 */

namespace Symfony\Component\CssSelector\Node;

/**
 * Represents a "<selector>:has(<subselector>)" node.
 *
 * This component is a port of the Python cssselect library,
 * which is copyright Ian Bicking, @see https://github.com/scrapy/cssselect.
 *
 * @author Franck Ranaivo-Harisoa <franckranaivo@gmail.com>
 *
 * @internal
 */
class RelationNode extends AbstractNode
{
    public function __construct(
        private NodeInterface $selector,
        private string $combinator,
        private NodeInterface $subSelector,
    ) {
    }

    public function getSelector(): NodeInterface
    {
        return $this->selector;
    }

    public function getCombinator(): string
    {
        return $this->combinator;
    }

    public function getSubSelector(): NodeInterface
    {
        return $this->subSelector;
    }

    public function getSpecificity(): Specificity
    {
        return $this->selector->getSpecificity()->plus($this->subSelector->getSpecificity());
    }

    public function __toString(): string
    {
        return \sprintf('%s[%s:has(%s)]', $this->getNodeName(), $this->selector, $this->subSelector);
    }
Repository: symfony/symfony
File: src/Symfony/Component/CssSelector/XPath/Extension/ExtensionInterface.php (PHP)

Code before:

    /**
     * Returns function translators.
     *
     * @return callable[]
     */
    public function getFunctionTranslators(): array;

    /**
     * Returns pseudo-class translators.
     *
     * @return callable[]
     */
    public function getPseudoClassTranslators(): array;

    /**
     * Returns attribute operation translators.
     *
     * @return callable[]
     */
    public function getAttributeMatchingTranslators(): array;

    /**
     * Returns relative combinators translators.
     *
     * @return callable[]
     */
    public function getRelativeCombinationTranslators(): array;

    /**
     * Returns extension name.
     */
    public function getName(): string;
}


Review comment (suggestion):
I suggest using `array<string, callable>` to describe the type of keys. Or even better, `array<string, callable(XPathExpr, XPathExpr): XPathExpr>` to describe the callable signature as well.

Code after:
     */
    public function getCombinationTranslators(): array;

    /**
     * Returns function translators.
     *
     * @return callable[]
     */
    public function getFunctionTranslators(): array;

    /**
     * Returns pseudo-class translators.
     *
     * @return callable[]
     */
    public function getPseudoClassTranslators(): array;

    /**
     * Returns attribute operation translators.
     *
     * @return callable[]
     */
    public function getAttributeMatchingTranslators(): array;

    /**
     * Returns combination translators found inside ":has()" relation.
     *
     * @return array<string, callable(XPathExpr, XPathExpr): XPathExpr>
     */
    public function getRelativeCombinationTranslators(): array;

    /**
     * Returns extension name.
     */
    public function getName(): string;
}
Repository: symfony/symfony
File: src/Symfony/Component/HttpKernel/Attribute/Serialize.php (PHP)

Code before:
<?php

/*
 * This file is part of the Symfony package.
 *
 * (c) Fabien Potencier <fabien@symfony.com>
 *
 * For the full copyright and license information, please view the LICENSE
 * file that was distributed with this source code.
 */

namespace Symfony\Component\HttpKernel\Attribute;

/**
 * Controller tag to serialize response.
 */
#[\Attribute(\Attribute::TARGET_METHOD)]
class Serialize
{
    public function __construct(
        public readonly int $code = 200,
        public readonly array $headers = [],
        public readonly array $context = [],
    ) {
    }
}


Review comment (suggestion):
you should add phpdoc describing the structure of those arrays (`$headers` is probably `array<string, string>` for instance)

Code after:
 * This file is part of the Symfony package.
 *
 * (c) Fabien Potencier <fabien@symfony.com>
 *
 * For the full copyright and license information, please view the LICENSE
 * file that was distributed with this source code.
 */

namespace Symfony\Component\HttpKernel\Attribute;

/**
 * Controller tag to serialize response.
 *
 * @author Konstantin Myakshin <molodchick@gmail.com>
 */
#[\Attribute(\Attribute::TARGET_METHOD)]
final class Serialize
{
    /**
     * @param int                  $code    The HTTP status code (200 "OK" by default)
     * @param array<string, mixed> $headers Extra headers to set on the response
     * @param array<string, mixed> $context The serialization context passed to the serializer
     */
    public function __construct(
        public readonly int $code = 200,
        public readonly array $headers = [],
        public readonly array $context = [],
    ) {
    }
}