Repository: denoland/deno
Pull request: fix(repl): use object_id for repl internal

close https://github.com/denoland/deno/issues/26599
close https://github.com/denoland/deno/issues/29121

This PR use object_id to interact with repl internal, instead of global variable (`__DENO_REPL_INTERNALS_{seconds}__`),
so we don't need to hide this in completions, etc.

Also add some error handling instead of simply `unwrap`.

## Before this PR

![image](https://github.com/user-attachments/assets/a0a49da8-3535-4927-aebd-541616866f33)
![image](https://github.com/user-attachments/assets/0f28d0f3-0cdb-42ba-a318-56f9116c6c27)

## After the PR

![image](https://github.com/user-attachments/assets/10bf873d-411c-4de0-91a6-a47d5b46469f)


![image](https://github.com/user-attachments/assets/0d76da6b-d74c-4648-8283-be193ccfe512)


btw, the the code in #26599 still create an error, may be we need to dive into this after merging this pr. 
![image](https://github.com/user-attachments/assets/4736769e-87c0-4631-9443-092a8b428709)
**UPDATE** `deno_console` seemly using `globalThis` for `Intl` and `Temporal` for prototype checks?
**UPDATE**: Open #29165 for this


Changed files:
filename: cli/tools/jupyter/server.rs
patch: @@ -36,7 +36,6 @@ use uuid::Uuid;
 
 use super::JupyterReplProxy;
 use crate::cdp;
-use crate::tools::repl;
 
 pub struct JupyterServer {
   execution_count: ExecutionCount,
@@ -322,11 +321,7 @@ impl JupyterServer {
             )
             .await
             .into_iter()
-            .filter(|n| {
-              !n.starts_with("Symbol(")
-                && n.starts_with(prop_name)
-                && n != &*repl::REPL_INTERNALS_NAME
-            })
+            .filter(|n| !n.starts_with("Symbol(") && n.starts_with(prop_name))
             .collect();
 
             if prop_name.len() > cursor_pos {
@@ -347,7 +342,7 @@ impl JupyterServer {
               get_global_lexical_scope_names(&mut self.repl_session_proxy)
                 .await,
             )
-            .filter(|n| n.starts_with(expr) && n != &*repl::REPL_INTERNALS_NAME)
+            .filter(|n| n.starts_with(expr))
             .collect::<Vec<_>>();
 
             // sort and remove duplicates

filename: cli/tools/repl/editor.rs
patch: @@ -38,7 +38,6 @@ use rustyline_derive::Helper;
 use rustyline_derive::Hinter;
 
 use super::channel::RustylineSyncMessageSender;
-use super::session::REPL_INTERNALS_NAME;
 use crate::cdp;
 use crate::colors;
 
@@ -209,11 +208,7 @@ impl Completer for EditorHelper {
       let candidates = self
         .get_expression_property_names(sub_expr)
         .into_iter()
-        .filter(|n| {
-          !n.starts_with("Symbol(")
-            && n.starts_with(prop_name)
-            && n != &*REPL_INTERNALS_NAME
-        })
+        .filter(|n| !n.starts_with("Symbol(") && n.starts_with(prop_name))
         .collect();
 
       Ok((pos - prop_name.len(), candidates))
@@ -223,7 +218,7 @@ impl Completer for EditorHelper {
         .get_expression_property_names("globalThis")
         .into_iter()
         .chain(self.get_global_lexical_scope_names())
-        .filter(|n| n.starts_with(expr) && n != &*REPL_INTERNALS_NAME)
+        .filter(|n| n.starts_with(expr))
         .collect::<Vec<_>>();
 
       // sort and remove duplicates

filename: cli/tools/repl/mod.rs
patch: @@ -35,7 +35,6 @@ use editor::ReplEditor;
 pub use session::EvaluationOutput;
 pub use session::ReplSession;
 pub use session::TsEvaluateResponse;
-pub use session::REPL_INTERNALS_NAME;
 
 use super::test::create_single_test_event_channel;
 

filename: cli/tools/repl/session.rs
patch: @@ -45,6 +45,7 @@ use tokio::sync::Mutex;
 use crate::args::deno_json::TsConfigResolver;
 use crate::args::CliOptions;
 use crate::cdp;
+use crate::cdp::RemoteObjectId;
 use crate::colors;
 use crate::lsp::ReplLanguageServer;
 use crate::npm::installer::NpmInstaller;
@@ -82,67 +83,54 @@ fn comment_source_to_position_range(
   }
 }
 
-/// We store functions used in the repl on this object because
-/// the user might modify the `Deno` global or delete it outright.
-pub static REPL_INTERNALS_NAME: Lazy<String> = Lazy::new(|| {
-  let now = std::time::SystemTime::now();
-  let seconds = now
-    .duration_since(std::time::SystemTime::UNIX_EPOCH)
-    .unwrap()
-    .as_secs();
-  // use a changing variable name to make it hard to depend on this
-  format!("__DENO_REPL_INTERNALS_{seconds}__")
-});
-
 fn get_prelude() -> String {
-  format!(
-    r#"
-Object.defineProperty(globalThis, "{0}", {{
-  enumerable: false,
-  writable: false,
-  value: {{
-    lastEvalResult: undefined,
-    lastThrownError: undefined,
-    inspectArgs: Deno[Deno.internal].inspectArgs,
-    noColor: Deno.noColor,
-    get closed() {{
-      return typeof globalThis.closed === 'undefined' ? false : globalThis.closed;
-    }}
-  }},
-}});
-Object.defineProperty(globalThis, "_", {{
-  configurable: true,
-  get: () => {0}.lastEvalResult,
-  set: (value) => {{
-   Object.defineProperty(globalThis, "_", {{
-     value: value,
-     writable: true,
-     enumerable: true,
-     configurable: true,
-   }});
-   console.log("Last evaluation result is no longer saved to _.");
-  }},
-}});
-
-Object.defineProperty(globalThis, "_error", {{
-  configurable: true,
-  get: () => {0}.lastThrownError,
-  set: (value) => {{
-   Object.defineProperty(globalThis, "_error", {{
-     value: value,
-     writable: true,
-     enumerable: true,
-     configurable: true,
-   }});
-
-   console.log("Last thrown error is no longer saved to _error.");
-  }},
-}});
-
-globalThis.clear = console.clear.bind(console);
-"#,
-    *REPL_INTERNALS_NAME
-  )
+  r#"(() => {
+  const repl_internal = {
+      lastEvalResult: undefined,
+      lastThrownError: undefined,
+      inspectArgs: Deno[Deno.internal].inspectArgs,
+      noColor: Deno.noColor,
+      get closed() {
+        try {
+          return typeof globalThis.closed === 'undefined' ? false : globalThis.closed;
+        } catch {
+          return false;
+        }
+      }
+    }
+  Object.defineProperty(globalThis, "_", {
+    configurable: true,
+    get: () => repl_internal.lastEvalResult,
+    set: (value) => {
+     Object.defineProperty(globalThis, "_", {
+       value: value,
+       writable: true,
+       enumerable: true,
+       configurable: true,
+     });
+     console.log("Last evaluation result is no longer saved to _.");
+    },
+  });
+
+  Object.defineProperty(globalThis, "_error", {
+    configurable: true,
+    get: () => repl_internal.lastThrownError,
+    set: (value) => {
+     Object.defineProperty(globalThis, "_error", {
+       value: value,
+       writable: true,
+       enumerable: true,
+       configurable: true,
+     });
+
+     console.log("Last thrown error is no longer saved to _error.");
+    },
+  });
+
+  globalThis.clear = console.clear.bind(console);
+
+  return repl_internal
+})()"#.to_string()
 }
 
 pub enum EvaluationOutput {
@@ -183,6 +171,7 @@ struct ReplJsxState {
 }
 
 pub struct ReplSession {
+  internal_object_id: Option<RemoteObjectId>,
   npm_installer: Option<Arc<NpmInstaller>>,
   resolver: Arc<CliResolver>,
   pub worker: MainWorker,
@@ -270,6 +259,7 @@ impl ReplSession {
       .transpile
       .use_ts_decorators;
     let mut repl_session = ReplSession {
+      internal_object_id: None,
       npm_installer,
       resolver,
       worker,
@@ -299,7 +289,8 @@ impl ReplSession {
     };
 
     // inject prelude
-    repl_session.evaluate_expression(&get_prelude()).await?;
+    let evaluated = repl_session.evaluate_expression(&get_prelude()).await?;
+    repl_session.internal_object_id = evaluated.result.object_id;
 
     Ok(repl_session)
   }
@@ -312,8 +303,13 @@ impl ReplSession {
   }
 
   pub async fn closing(&mut self) -> Result<bool, AnyError> {
-    let expression = format!(r#"{}.closed"#, *REPL_INTERNALS_NAME);
-    let result = self.evaluate_expression(&expression).await?.result;
+    let result = self
+      .call_function_on_repl_internal_obj(
+        r#"function () { return this.closed; }"#.to_string(),
+        &[],
+      )
+      .await?
+      .result;
     let closed = result
       .value
       .ok_or_else(|| anyhow!(result.description.unwrap()))?
@@ -500,18 +496,17 @@ impl ReplSession {
       .post_message_with_event_loop(
         "Runtime.callFunctionOn",
         Some(cdp::CallFunctionOnArgs {
-          function_declaration: format!(
-            r#"function (object) {{ {}.lastThrownError = object; }}"#,
-            *REPL_INTERNALS_NAME
-          ),
-          object_id: None,
+          function_declaration:
+            r#"function (object) { this.lastThrownError = object; }"#
+              .to_string(),
+          object_id: self.internal_object_id.clone(),
           arguments: Some(vec![error.into()]),
           silent: None,
           return_by_value: None,
           generate_preview: None,
           user_gesture: None,
           await_promise: None,
-          execution_context_id: Some(self.context_id),
+          execution_context_id: None,
           object_group: None,
           throw_on_side_effect: None,
         }),
@@ -528,18 +523,15 @@ impl ReplSession {
       .post_message_with_event_loop(
         "Runtime.callFunctionOn",
         Some(cdp::CallFunctionOnArgs {
-          function_declaration: format!(
-            r#"function (object) {{ {}.lastEvalResult = object; }}"#,
-            *REPL_INTERNALS_NAME
-          ),
-          object_id: None,
+          function_declaration: r#"function (object) { this.lastEvalResult = object; }"#.to_string(),
+          object_id: self.internal_object_id.clone(),
           arguments: Some(vec![evaluate_result.into()]),
           silent: None,
           return_by_value: None,
           generate_preview: None,
           user_gesture: None,
           await_promise: None,
-          execution_context_id: Some(self.context_id),
+          execution_context_id: None,
           object_group: None,
           throw_on_side_effect: None,
         }),
@@ -583,6 +575,41 @@ impl ReplSession {
     Ok(response)
   }
 
+  pub async fn call_function_on_repl_internal_obj(
+    &mut self,
+    function_declaration: String,
+    args: &[cdp::RemoteObject],
+  ) -> Result<cdp::CallFunctionOnResponse, AnyError> {
+    let arguments: Option<Vec<cdp::CallArgument>> = if args.is_empty() {
+      None
+    } else {
+      Some(args.iter().map(|a| a.into()).collect())
+    };
+
+    let inspect_response = self
+      .post_message_with_event_loop(
+        "Runtime.callFunctionOn",
+        Some(cdp::CallFunctionOnArgs {
+          function_declaration,
+          object_id: self.internal_object_id.clone(),
+          arguments,
+          silent: None,
+          return_by_value: None,
+          generate_preview: None,
+          user_gesture: None,
+          await_promise: None,
+          execution_context_id: None,
+          object_group: None,
+          throw_on_side_effect: None,
+        }),
+      )
+      .await?;
+
+    let response: cdp::CallFunctionOnResponse =
+      serde_json::from_value(inspect_response)?;
+    Ok(response)
+  }
+
   pub async fn get_eval_value(
     &mut self,
     evaluate_result: &cdp::RemoteObject,
@@ -591,24 +618,26 @@ impl ReplSession {
     // consistent with the previous implementation we just get the preview result from
     // Deno.inspectArgs.
     let response = self
-      .call_function_on_args(
-        format!(
-          r#"function (object) {{
-          try {{
-            return {0}.inspectArgs(["%o", object], {{ colors: !{0}.noColor }});
-          }} catch (err) {{
-            return {0}.inspectArgs(["%o", err]);
-          }}
-        }}"#,
-          *REPL_INTERNALS_NAME
-        ),
+      .call_function_on_repl_internal_obj(
+        r#"function (object) {
+          try {
+            return this.inspectArgs(["%o", object], { colors: !this.noColor });
+          } catch (err) {
+            return this.inspectArgs(["%o", err]);
+          }
+        }"#
+          .to_string(),
         &[evaluate_result.clone()],
       )
       .await?;
-    let value = response.result.value.unwrap();
-    let s = value.as_str().unwrap();
+    let s = response
+      .result
+      .value
+      .map(|v| v.as_str().unwrap().to_string())
+      .or(response.result.description)
+      .ok_or_else(|| anyhow!("failed to evaluate expression"))?;
 
-    Ok(s.to_string())
+    Ok(s)
   }
 
   async fn evaluate_ts_expression(

filename: tests/integration/repl_tests.rs
patch: @@ -1169,3 +1169,19 @@ fn repl_json_imports() {
       console.expect(r#"{ hello: "world" }"#);
     });
 }
+
+#[test]
+fn repl_no_globalthis() {
+  let context = TestContextBuilder::default().use_temp_cwd().build();
+  context
+    .new_command()
+    .env("NO_COLOR", "1")
+    .args_vec(["repl", "-A"])
+    .with_pty(|mut console| {
+      console.write_line_raw("delete globalThis.globalThis;");
+      console.expect("true");
+      console.write_line_raw("console.log('Hello World')");
+      console.expect(r#"Hello World"#);
+      console.expect(r#"undefined"#);
+    });
+}