Fix versioning issue when @added is used implicitly and explicitly

.chronus/changes/versioning-FixVersioningBugs-2024-4-22-20-42-27.md

@@ -0,0 +1,8 @@
+---
+# Change versionKind to one of: internal, fix, dependencies, feature, deprecation, breaking
+changeKind: fix
+packages:
+  - "@typespec/versioning"
+---
+
+If a property were marked with @added on a later version, the logic that said it was originally added on the first version was erroneously removed, resulting in incorrect projections.

packages/versioning/src/versioning.ts

@@ -1,5 +1,11 @@
-import type { Enum, EnumMember, Namespace, Program, Type } from "@typespec/compiler";
-import { compilerAssert, getNamespaceFullName } from "@typespec/compiler";
+import {
+  getNamespaceFullName,
+  type Enum,
+  type EnumMember,
+  type Namespace,
+  type Program,
+  type Type,
+} from "@typespec/compiler";
 import {
   getAddedOnVersions,
   getRemovedOnVersions,
@@ -189,6 +195,55 @@ export enum Availability {
   Removed = "Removed",
 }
 
+function getParentAddedVersion(
+  program: Program,
+  type: Type,
+  versions: Version[]
+): Version | undefined {
+  let parentMap: Map<string, Availability> | undefined = undefined;
+  if (type.kind === "ModelProperty" && type.model !== undefined) {
+    parentMap = getAvailabilityMap(program, type.model);
+  } else if (type.kind === "Operation" && type.interface !== undefined) {
+    parentMap = getAvailabilityMap(program, type.interface);
+  }
+  if (parentMap === undefined) return undefined;
+  for (const [key, value] of parentMap.entries()) {
+    if (value === Availability.Added) {
+      return versions.find((x) => x.name === key);
+    }
+  }
+  return undefined;
+}
+
+function getParentAddedVersionInTimeline(
+  program: Program,
+  type: Type,
+  timeline: VersioningTimeline
+): Version | undefined {
+  let parentMap: Map<TimelineMoment, Availability> | undefined = undefined;
+  if (type.kind === "ModelProperty" && type.model !== undefined) {
+    parentMap = getAvailabilityMapInTimeline(program, type.model, timeline);
+  } else if (type.kind === "Operation" && type.interface !== undefined) {
+    parentMap = getAvailabilityMapInTimeline(program, type.interface, timeline);
+  }
+  if (parentMap === undefined) return undefined;
+  for (const [moment, availability] of parentMap.entries()) {
+    if (availability === Availability.Added) {
+      return moment.versions().next().value;
+    }
+  }
+  return undefined;
+}
+
+/**
+ * Returns true if the first version modifier was @added, false if @removed.
+ */
+function resolveAddedFirst(added: Version[], removed: Version[]): boolean {
+  if (added.length === 0) return false;
+  if (removed.length === 0) return true;
+  return added[0].index < removed[0].index;
+}
+
 export function getAvailabilityMap(
   program: Program,
   type: Type
@@ -199,7 +254,8 @@ export function getAvailabilityMap(
   // if unversioned then everything exists
   if (allVersions === undefined) return undefined;
 
-  const added = getAddedOnVersions(program, type) ?? [];
+  const parentAdded = getParentAddedVersion(program, type, allVersions);
+  let added = getAddedOnVersions(program, type) ?? [];
   const removed = getRemovedOnVersions(program, type) ?? [];
   const typeChanged = getTypeChangedFrom(program, type);
   const returnTypeChanged = getReturnTypeChangedFrom(program, type);
@@ -214,27 +270,23 @@ export function getAvailabilityMap(
   )
     return undefined;
 
-  let parentMap: Map<string, Availability> | undefined = undefined;
-  if (type.kind === "ModelProperty" && type.model !== undefined) {
-    parentMap = getAvailabilityMap(program, type.model);
-  } else if (type.kind === "Operation" && type.interface !== undefined) {
-    parentMap = getAvailabilityMap(program, type.interface);
-  }
+  const wasAddedFirst = resolveAddedFirst(added, removed);
 
   // implicitly, all versioned things are assumed to have been added at
-  // v1 if not specified
-  if (!added.length) {
-    if (parentMap !== undefined) {
-      parentMap.forEach((key, value) => {
-        if (key === Availability.Added.valueOf()) {
-          const match = allVersions.find((x) => x.name === value);
-          compilerAssert(match !== undefined, "Version not found");
-          added.push(match);
-        }
-      });
-    } else {
-      added.push(allVersions[0]);
-    }
+  // v1 if not specified, or inherited from their parent.
+  if (!wasAddedFirst && !parentAdded) {
+    // if the first version modifier was @removed, and the parent is implicitly available,
+    // then assume the type was available.
+    added = [allVersions[0], ...added];
+  } else if (!added.length && !parentAdded) {
+    // no version information on the item or its parent implicitly means it has always been available
+    added.push(allVersions[0]);
+  } else if (!added.length && parentAdded) {
+    // if no version info on type but is on parent, inherit that parent's "added" version
+    added.push(parentAdded);
+  } else if (added.length && parentAdded) {
+    // if "added" info on both the type and parent, combine them
+    added = [parentAdded, ...added];
   }
 
   // something isn't available by default
@@ -264,7 +316,8 @@ export function getAvailabilityMapInTimeline(
 ): Map<TimelineMoment, Availability> | undefined {
   const avail = new Map<TimelineMoment, Availability>();
 
-  const added = getAddedOnVersions(program, type) ?? [];
+  const parentAdded = getParentAddedVersionInTimeline(program, type, timeline);
+  let added = getAddedOnVersions(program, type) ?? [];
   const removed = getRemovedOnVersions(program, type) ?? [];
   const typeChanged = getTypeChangedFrom(program, type);
   const returnTypeChanged = getReturnTypeChangedFrom(program, type);
@@ -279,10 +332,24 @@ export function getAvailabilityMapInTimeline(
   )
     return undefined;
 
+  const wasAddedFirst = resolveAddedFirst(added, removed);
+
   // implicitly, all versioned things are assumed to have been added at
-  // v1 if not specified
-  if (!added.length) {
-    added.push(timeline.first().versions().next().value);
+  // v1 if not specified, or inherited from their parent.
+  const firstVersion = timeline.first().versions().next().value;
+  if (!wasAddedFirst && !parentAdded) {
+    // if the first version modifier was @removed, and the parent is implicitly available,
+    // then assume the type was available.
+    added = [firstVersion, ...added];
+  } else if (!added.length && !parentAdded) {
+    // no version information on the item or its parent implicitly means it has always been available
+    added.push(firstVersion);
+  } else if (!added.length && parentAdded) {
+    // if no version info on type but is on parent, inherit that parent's "added" version
+    added.push(parentAdded);
+  } else if (added.length && parentAdded) {
+    // if "added" info on both the type and parent, combine them
+    added = [parentAdded, ...added];
   }
 
   // something isn't available by default

packages/versioning/test/versioning.test.ts

@@ -281,7 +281,7 @@ describe("versioning: logic", () => {
       );
     });
 
-    it("can be removed respecting model versioning", async () => {
+    it("can be removed respecting model versioning with explicit versions", async () => {
       const {
         source,
         projections: [v2, v3, v4],
@@ -290,20 +290,51 @@ describe("versioning: logic", () => {
         `@added(Versions.v2)
         model Test {
           a: int32;
-          @removed(Versions.v3) b: int32;
+          @removed(Versions.v3)
+          @added(Versions.v4)
+          b: int32;
         }
         `
       );
 
       assertHasProperties(v2, ["a", "b"]);
       assertHasProperties(v3, ["a"]);
-      assertHasProperties(v4, ["a"]);
+      assertHasProperties(v4, ["a", "b"]);
+
+      assertModelProjectsTo(
+        [
+          [v2, "v2"],
+          [v3, "v3"],
+          [v4, "v4"],
+        ],
+        source
+      );
+    });
+
+    it("can be removed respecting model versioning with implicit versions", async () => {
+      const {
+        source,
+        projections: [v1, v2, v3],
+      } = await versionedModel(
+        ["v1", "v2", "v3"],
+        `model Test {
+          a: int32;
+          @removed(Versions.v2)
+          @added(Versions.v3)
+          b: int32;
+        }
+        `
+      );
+
+      assertHasProperties(v1, ["a", "b"]);
+      assertHasProperties(v2, ["a"]);
+      assertHasProperties(v3, ["a", "b"]);
 
       assertModelProjectsTo(
         [
+          [v1, "v1"],
           [v2, "v2"],
           [v3, "v3"],
-          [v3, "v4"],
         ],
         source
       );
@@ -1403,12 +1434,15 @@ describe("versioning: logic", () => {
         `@added(Versions.v2)
         interface Test {
           allVersions(): void;
-          @removed(Versions.v3) version2Only(): void;
+          @removed(Versions.v3) 
+          @added(Versions.v4)
+          foo(): void;
         }
         `
       );
-      assertHasOperations(v2, ["allVersions", "version2Only"]);
+      assertHasOperations(v2, ["allVersions", "foo"]);
       assertHasOperations(v3, ["allVersions"]);
+      assertHasOperations(v4, ["allVersions", "foo"]);
       assertInterfaceProjectsTo(
         [
           [v2, "v2"],