Skip to content

Commit c8186a4

Browse files
owenlin0owenlin0
committed
[app-server] set '<field_name>: null' instead of omitting the field entirely
1 parent f4f9695 commit c8186a4

File tree

13 files changed

+246
-182
lines changed

13 files changed

+246
-182
lines changed

‎codex-rs/app-server-protocol/src/export.rs‎

Lines changed: 88 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -545,7 +545,7 @@ mod tests {
545545
use uuid::Uuid;
546546

547547
#[test]
548-
fn generated_ts_omits_undefined_unions_for_optionals() -> Result<()> {
548+
fn generated_ts_has_no_optional_nullable_fields() -> Result<()> {
549549
let output_dir = std::env::temp_dir().join(format!("codex_ts_types_{}", Uuid::now_v7()));
550550
fs::create_dir(&output_dir)?;
551551

@@ -562,7 +562,7 @@ mod tests {
562562
generate_ts(&output_dir, None)?;
563563

564564
let mut undefined_offenders = Vec::new();
565-
let mut missing_optional_marker = BTreeSet::new();
565+
let mut optional_nullable_offenders = BTreeSet::new();
566566
let mut stack = vec![output_dir];
567567
while let Some(dir) = stack.pop() {
568568
for entry in fs::read_dir(&dir)? {
@@ -591,27 +591,80 @@ mod tests {
591591
let mut search_start = 0;
592592
while let Some(idx) = contents[search_start..].find("| null") {
593593
let abs_idx = search_start + idx;
594-
let Some(colon_idx) = contents[..abs_idx].rfind(':') else {
595-
search_start = abs_idx + 5;
596-
continue;
597-
};
594+
// Find the property-colon for this field by scanning forward
595+
// from the start of the segment and ignoring nested braces,
596+
// brackets, and parens. This avoids colons inside nested
597+
// type literals like `{ [k in string]?: string }`.
598598

599-
let line_start_idx = contents[..colon_idx]
600-
.rfind('\n')
601-
.map(|i| i + 1)
602-
.unwrap_or(0);
599+
let line_start_idx =
600+
contents[..abs_idx].rfind('\n').map(|i| i + 1).unwrap_or(0);
603601

604602
let mut segment_start_idx = line_start_idx;
605-
if let Some(rel_idx) = contents[line_start_idx..colon_idx].rfind(',') {
603+
if let Some(rel_idx) = contents[line_start_idx..abs_idx].rfind(',') {
606604
segment_start_idx = segment_start_idx.max(line_start_idx + rel_idx + 1);
607605
}
608-
if let Some(rel_idx) = contents[line_start_idx..colon_idx].rfind('{') {
606+
if let Some(rel_idx) = contents[line_start_idx..abs_idx].rfind('{') {
609607
segment_start_idx = segment_start_idx.max(line_start_idx + rel_idx + 1);
610608
}
611-
if let Some(rel_idx) = contents[line_start_idx..colon_idx].rfind('}') {
609+
if let Some(rel_idx) = contents[line_start_idx..abs_idx].rfind('}') {
612610
segment_start_idx = segment_start_idx.max(line_start_idx + rel_idx + 1);
613611
}
614612

613+
// Scan forward for the colon that separates the field name from its type.
614+
let mut level_brace = 0_i32;
615+
let mut level_brack = 0_i32;
616+
let mut level_paren = 0_i32;
617+
let mut in_single = false;
618+
let mut in_double = false;
619+
let mut escape = false;
620+
let mut prop_colon_idx = None;
621+
for (i, ch) in contents[segment_start_idx..abs_idx].char_indices() {
622+
let idx_abs = segment_start_idx + i;
623+
if escape {
624+
escape = false;
625+
continue;
626+
}
627+
match ch {
628+
'\\' => {
629+
// Only treat as escape when inside a string.
630+
if in_single || in_double {
631+
escape = true;
632+
}
633+
}
634+
'\'' => {
635+
if !in_double {
636+
in_single = !in_single;
637+
}
638+
}
639+
'"' => {
640+
if !in_single {
641+
in_double = !in_double;
642+
}
643+
}
644+
'{' if !in_single && !in_double => level_brace += 1,
645+
'}' if !in_single && !in_double => level_brace -= 1,
646+
'[' if !in_single && !in_double => level_brack += 1,
647+
']' if !in_single && !in_double => level_brack -= 1,
648+
'(' if !in_single && !in_double => level_paren += 1,
649+
')' if !in_single && !in_double => level_paren -= 1,
650+
':' if !in_single
651+
&& !in_double
652+
&& level_brace == 0
653+
&& level_brack == 0
654+
&& level_paren == 0 =>
655+
{
656+
prop_colon_idx = Some(idx_abs);
657+
break;
658+
}
659+
_ => {}
660+
}
661+
}
662+
663+
let Some(colon_idx) = prop_colon_idx else {
664+
search_start = abs_idx + 5;
665+
continue;
666+
};
667+
615668
let mut field_prefix = contents[segment_start_idx..colon_idx].trim();
616669
if field_prefix.is_empty() {
617670
search_start = abs_idx + 5;
@@ -640,25 +693,26 @@ mod tests {
640693
continue;
641694
}
642695

696+
// If the last non-whitespace before ':' is '?', then this is an
697+
// optional field with a nullable type (i.e., "?: T | null"),
698+
// which we explicitly disallow.
643699
if field_prefix.chars().rev().find(|c| !c.is_whitespace()) == Some('?') {
644-
search_start = abs_idx + 5;
645-
continue;
700+
let line_number =
701+
contents[..abs_idx].chars().filter(|c| *c == '\n').count() + 1;
702+
let offending_line_end = contents[line_start_idx..]
703+
.find('\n')
704+
.map(|i| line_start_idx + i)
705+
.unwrap_or(contents.len());
706+
let offending_snippet =
707+
contents[line_start_idx..offending_line_end].trim();
708+
709+
optional_nullable_offenders.insert(format!(
710+
"{}:{}: {offending_snippet}",
711+
path.display(),
712+
line_number
713+
));
646714
}
647715

648-
let line_number =
649-
contents[..abs_idx].chars().filter(|c| *c == '\n').count() + 1;
650-
let offending_line_end = contents[line_start_idx..]
651-
.find('\n')
652-
.map(|i| line_start_idx + i)
653-
.unwrap_or(contents.len());
654-
let offending_snippet = contents[line_start_idx..offending_line_end].trim();
655-
656-
missing_optional_marker.insert(format!(
657-
"{}:{}: {offending_snippet}",
658-
path.display(),
659-
line_number
660-
));
661-
662716
search_start = abs_idx + 5;
663717
}
664718
}
@@ -670,12 +724,12 @@ mod tests {
670724
"Generated TypeScript still includes unions with `undefined` in {undefined_offenders:?}"
671725
);
672726

673-
// If this test fails, it means that a struct field that is `Option<T>` in Rust
674-
// is being generated as `T | null` in TypeScript, without the optional marker
675-
// (`?`). To fix this, add #[ts(optional_fields = nullable)] to the struct definition.
727+
// If this assertion fails, it means a field was generated as
728+
// "?: T | null" — i.e., both optional (undefined) and nullable (null).
729+
// We only want either "?: T" or ": T | null".
676730
assert!(
677-
missing_optional_marker.is_empty(),
678-
"Generated TypeScript has nullable fields without an optional marker: {missing_optional_marker:?}"
731+
optional_nullable_offenders.is_empty(),
732+
"Generated TypeScript has optional fields with nullable types (disallowed '?: T | null'), add #[ts(optional)] to fix:\n{optional_nullable_offenders:?}"
679733
);
680734

681735
Ok(())

‎codex-rs/app-server-protocol/src/jsonrpc_lite.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,20 +30,20 @@ pub enum JSONRPCMessage {
3030

3131
/// A request that expects a response.
3232
#[derive(Debug, Clone, PartialEq, Deserialize, Serialize, JsonSchema, TS)]
33-
#[ts(optional_fields = nullable)]
3433
pub struct JSONRPCRequest {
3534
pub id: RequestId,
3635
pub method: String,
3736
#[serde(default, skip_serializing_if = "Option::is_none")]
37+
#[ts(optional)]
3838
pub params: Option<serde_json::Value>,
3939
}
4040

4141
/// A notification which does not expect a response.
4242
#[derive(Debug, Clone, PartialEq, Deserialize, Serialize, JsonSchema, TS)]
43-
#[ts(optional_fields = nullable)]
4443
pub struct JSONRPCNotification {
4544
pub method: String,
4645
#[serde(default, skip_serializing_if = "Option::is_none")]
46+
#[ts(optional)]
4747
pub params: Option<serde_json::Value>,
4848
}
4949

@@ -62,10 +62,10 @@ pub struct JSONRPCError {
6262
}
6363

6464
#[derive(Debug, Clone, PartialEq, Deserialize, Serialize, JsonSchema, TS)]
65-
#[ts(optional_fields = nullable)]
6665
pub struct JSONRPCErrorError {
6766
pub code: i64,
6867
#[serde(default, skip_serializing_if = "Option::is_none")]
68+
#[ts(optional)]
6969
pub data: Option<serde_json::Value>,
7070
pub message: String,
7171
}

0 commit comments

Comments
 (0)