From efd743339c8c75bacd7ebb283246830ccb6a5e50 Mon Sep 17 00:00:00 2001
From: Martin von Zweigbergk <martinvonz@google.com>
Date: Fri, 5 May 2023 15:50:37 -0700
Subject: [PATCH] revset: don't allow symbols in `RevsetExpression::resolve()`

When creating `RevsetExpression` programmatically, I think we should
use commit ids instead of symbols in the expression. This commit adds
a check for that by using a `SymbolResolver` that always errors
out.
---
 lib/src/revset.rs        | 14 +++++++++++++-
 lib/tests/test_revset.rs | 11 ++++++++---
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/lib/src/revset.rs b/lib/src/revset.rs
index 076338b85..964774450 100644
--- a/lib/src/revset.rs
+++ b/lib/src/revset.rs
@@ -436,7 +436,7 @@ impl RevsetExpression {
         self: Rc<Self>,
         repo: &dyn Repo,
     ) -> Result<ResolvedExpression, RevsetResolutionError> {
-        let symbol_resolver = DefaultSymbolResolver::new(repo, None);
+        let symbol_resolver = FailingSymbolResolver;
         resolve_symbols(repo, self, &symbol_resolver)
             .map(|expression| resolve_visibility(repo, &expression))
     }
@@ -1669,6 +1669,18 @@ pub trait SymbolResolver {
     fn resolve_symbol(&self, symbol: &str) -> Result<Vec<CommitId>, RevsetResolutionError>;
 }
 
+/// Fails on any attempt to resolve a symbol.
+pub struct FailingSymbolResolver;
+
+impl SymbolResolver for FailingSymbolResolver {
+    fn resolve_symbol(&self, symbol: &str) -> Result<Vec<CommitId>, RevsetResolutionError> {
+        Err(RevsetResolutionError::NoSuchRevision(format!(
+            "Won't resolve symbol {symbol:?}. When creating revsets programmatically, avoid using \
+             RevsetExpression::symbol(); use RevsetExpression::commits() instead."
+        )))
+    }
+}
+
 /// Resolves the "root" and "@" symbols, branches, remote branches, tags, git
 /// refs, and full and abbreviated commit and change ids.
 pub struct DefaultSymbolResolver<'a> {
diff --git a/lib/tests/test_revset.rs b/lib/tests/test_revset.rs
index 3899b9f5a..a4c72689d 100644
--- a/lib/tests/test_revset.rs
+++ b/lib/tests/test_revset.rs
@@ -40,8 +40,9 @@ fn revset_for_commits<'index>(
     repo: &'index dyn Repo,
     commits: &[&Commit],
 ) -> Box<dyn Revset<'index> + 'index> {
+    let symbol_resolver = DefaultSymbolResolver::new(repo, None);
     RevsetExpression::commits(commits.iter().map(|commit| commit.id().clone()).collect())
-        .resolve(repo)
+        .resolve_user_expression(repo, &symbol_resolver)
         .unwrap()
         .evaluate(repo)
         .unwrap()
@@ -173,8 +174,9 @@ fn test_resolve_symbol_commit_id() {
 
     // Test present() suppresses only NoSuchRevision error
     assert_eq!(resolve_commit_ids(repo.as_ref(), "present(foo)"), []);
+    let symbol_resolver = DefaultSymbolResolver::new(repo.as_ref(), None);
     assert_matches!(
-        optimize(parse("present(04)", &RevsetAliasesMap::new(), None).unwrap()).resolve(repo.as_ref()),
+        optimize(parse("present(04)", &RevsetAliasesMap::new(), None).unwrap()).resolve_user_expression(repo.as_ref(), &symbol_resolver),
         Err(RevsetResolutionError::AmbiguousIdPrefix(s)) if s == "04"
     );
     assert_eq!(
@@ -504,7 +506,10 @@ fn test_resolve_symbol_git_refs() {
 
 fn resolve_commit_ids(repo: &dyn Repo, revset_str: &str) -> Vec<CommitId> {
     let expression = optimize(parse(revset_str, &RevsetAliasesMap::new(), None).unwrap());
-    let expression = expression.resolve(repo).unwrap();
+    let symbol_resolver = DefaultSymbolResolver::new(repo, None);
+    let expression = expression
+        .resolve_user_expression(repo, &symbol_resolver)
+        .unwrap();
     expression.evaluate(repo).unwrap().iter().collect()
 }