Skip to content

refactor: Naming cleanup #2363

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
iwoplaza wants to merge 1 commit into
base: main
Choose a base branch
from
Open

refactor: Naming cleanup #2363

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/typegpu/src/core/buffer/bufferUsage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ class TgpuFixedBufferImpl<TData extends BaseData, TUsage extends BindableBufferU

[$resolve](ctx: ResolutionCtx): ResolvedSnippet {
const dataType = this.buffer.dataType;
const id = ctx.getUniqueName(this);
const id = ctx.makeUniqueIdentifier(getName(this), 'global');
const { group, binding } = ctx.allocateFixedEntry(
this.usage === 'uniform' ? { uniform: dataType } : { storage: dataType, access: this.usage },
this.buffer,
Expand Down Expand Up @@ -241,7 +241,7 @@ export class TgpuLaidOutBufferImpl<TData extends BaseData, TUsage extends Bindab
}

[$resolve](ctx: ResolutionCtx): ResolvedSnippet {
const id = ctx.getUniqueName(this);
const id = ctx.makeUniqueIdentifier(getName(this), 'global');
const group = ctx.allocateLayoutEntry(this.#membership.layout);
const usage = usageToVarTemplateMap[this.usage];

Expand Down
2 changes: 1 addition & 1 deletion packages/typegpu/src/core/constant/tgpuConstant.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ class TgpuConstImpl<TDataType extends BaseData> implements TgpuConst<TDataType>,
}

[$resolve](ctx: ResolutionCtx): ResolvedSnippet {
const id = ctx.getUniqueName(this);
const id = ctx.makeUniqueIdentifier(getName(this), 'global');
const resolvedDataType = ctx.resolve(this.dataType).value;
const resolvedValue = ctx.resolve(this.#value, this.dataType).value;

Expand Down
30 changes: 19 additions & 11 deletions packages/typegpu/src/core/function/fnCore.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { undecorate } from '../../data/dataTypes.ts';
import { type ResolvedSnippet, snip } from '../../data/snippet.ts';
import { type BaseData, isWgslData, isWgslStruct, Void } from '../../data/wgslTypes.ts';
import { MissingLinksError } from '../../errors.ts';
import { validateIdentifier } from '../../nameUtils.ts';
import { getMetaData, getName } from '../../shared/meta.ts';
import { $getNameForward } from '../../shared/symbols.ts';
import type { ResolutionCtx, TgpuShaderStage } from '../../types.ts';
Expand Down Expand Up @@ -74,37 +75,44 @@ export function createFnCore(
applyExternals(externalMap, externals);
}

const id = ctx.getUniqueName(this);
const id = ctx.makeUniqueIdentifier(getName(this), 'global');

if (typeof implementation === 'string') {
if (!returnType) {
throw new Error('Explicit return type is required for string implementation');
}

const validArgNames = entryInput
? Object.fromEntries(
entryInput.positionalArgs.map((a) => [a.schemaKey, ctx.makeNameValid(a.schemaKey)]),
)
: undefined;
if (entryInput) {
for (const arg of entryInput.positionalArgs) {
const result = validateIdentifier(arg.schemaKey);
if (!result.success) {
throw new Error(
`Invalid argument name "${arg.schemaKey}"${result.error ? `: ${result.error}` : ''}`,
);
}
}

if (validArgNames && Object.keys(validArgNames).length > 0) {
applyExternals(externalMap, { in: validArgNames });
applyExternals(externalMap, {
in: Object.fromEntries(
entryInput.positionalArgs.map((a) => [a.schemaKey, a.schemaKey]),
),
});
}
Comment on lines +85 to 100
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isValidIdentifier can throw (e.g. whitespace / leading underscores), so if (!isValidIdentifier(...)) will not reliably produce the intended Invalid argument name: ... error; it will often throw a different error message instead. Also, this is a behavior change from the previous ctx.makeNameValid(...) approach: previously, schema keys that weren't safe WGSL identifiers could be mapped/renamed; now they hard-error. If the intent is only to improve error reporting, wrap validation in a try/catch and rethrow with a consistent message. If the intent is to preserve prior behavior, reintroduce a mapping step (generate safe identifiers and pass that mapping to applyExternals) rather than requiring schemaKey to already be usable as a WGSL parameter name.

Copilot uses AI. Check for mistakes.

const replacedImpl = replaceExternalsInWgsl(ctx, externalMap, implementation);

let header = '';
let body = '';

if (functionType !== 'normal' && entryInput && validArgNames) {
if (functionType !== 'normal' && entryInput) {
const { dataSchema, positionalArgs } = entryInput;
const parts: string[] = [];
if (dataSchema && isArgUsedInBody('in', replacedImpl)) {
parts.push(`in: ${ctx.resolve(dataSchema).value}`);
}
for (const a of positionalArgs) {
const argName = validArgNames[a.schemaKey] ?? '';
if (argName !== '' && isArgUsedInBody(argName, replacedImpl)) {
const argName = a.schemaKey;
if (isArgUsedInBody(argName, replacedImpl)) {
parts.push(`${getAttributesString(a.type)}${argName}: ${ctx.resolve(a.type).value}`);
}
}
Expand Down
55 changes: 7 additions & 48 deletions packages/typegpu/src/core/resolve/namespace.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
import type { ResolvedSnippet } from '../../data/snippet.ts';
import { type NameRegistry, RandomNameRegistry, StrictNameRegistry } from '../../nameRegistry.ts';
import { getName } from '../../shared/meta.ts';
import { bannedTokens, builtins } from '../../nameUtils.ts';
import { $internal } from '../../shared/symbols.ts';
import { ShelllessRepository } from '../../tgsl/shellless.ts';
import type { TgpuLazy, TgpuSlot } from '../slot/slotTypes.ts';

type SlotToValueMap = Map<TgpuSlot<unknown>, unknown>;

export interface NamespaceInternal {
readonly nameRegistry: NameRegistry;
readonly takenGlobalIdentifiers: Set<string>;
readonly shelllessRepo: ShelllessRepository;
readonly strategy: 'random' | 'strict';

memoizedResolves: WeakMap<
// WeakMap because if the item does not exist anymore,
Expand All @@ -24,73 +24,32 @@ export interface NamespaceInternal {
TgpuLazy<unknown>,
{ slotToValueMap: SlotToValueMap; result: unknown }[]
>;

listeners: {
[K in keyof NamespaceEventMap]: Set<(event: NamespaceEventMap[K]) => void>;
};
}

type NamespaceEventMap = {
name: { target: object; name: string };
};

type DetachListener = () => void;

export interface Namespace {
readonly [$internal]: NamespaceInternal;

on<TEvent extends keyof NamespaceEventMap>(
event: TEvent,
listener: (event: NamespaceEventMap[TEvent]) => void,
): DetachListener;
}

class NamespaceImpl implements Namespace {
readonly [$internal]: NamespaceInternal;

constructor(nameRegistry: NameRegistry) {
constructor(strategy: 'random' | 'strict') {
this[$internal] = {
nameRegistry,
strategy,
takenGlobalIdentifiers: new Set([...bannedTokens, ...builtins]),
shelllessRepo: new ShelllessRepository(),
memoizedResolves: new WeakMap(),
memoizedLazy: new WeakMap(),
listeners: {
name: new Set(),
},
};
}

on<TEvent extends keyof NamespaceEventMap>(
event: TEvent,
listener: (event: NamespaceEventMap[TEvent]) => void,
): DetachListener {
if (event === 'name') {
const listeners = this[$internal].listeners.name;
listeners.add(listener);

return () => listeners.delete(listener);
}

throw new Error(`Unsupported event: ${event}`);
}
}

export interface NamespaceOptions {
names?: 'random' | 'strict' | undefined;
}

export function getUniqueName(namespace: NamespaceInternal, resource: object): string {
const name = namespace.nameRegistry.makeUnique(getName(resource), true);
for (const listener of namespace.listeners.name) {
listener({ target: resource, name });
}
return name;
}

export function namespace(options?: NamespaceOptions): Namespace {
const { names = 'strict' } = options ?? {};

return new NamespaceImpl(
names === 'strict' ? new StrictNameRegistry() : new RandomNameRegistry(),
);
return new NamespaceImpl(names);
}
5 changes: 3 additions & 2 deletions packages/typegpu/src/core/resolve/resolveData.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import type {
WgslArray,
WgslStruct,
} from '../../data/wgslTypes.ts';
import { getName } from '../../shared/meta.ts';
import { $internal } from '../../shared/symbols.ts';
import { assertExhaustive } from '../../shared/utilityTypes.ts';
import type { ResolutionCtx } from '../../types.ts';
Expand Down Expand Up @@ -127,7 +128,7 @@ function resolveStruct(ctx: ResolutionCtx, struct: WgslStruct) {
if (struct[$internal].isAbstruct) {
throw new Error('Cannot resolve abstract struct types to WGSL.');
}
const id = ctx.getUniqueName(struct);
const id = ctx.makeUniqueIdentifier(getName(struct), 'global');

ctx.addDeclaration(`\
struct ${id} {
Expand Down Expand Up @@ -155,7 +156,7 @@ ${Object.entries(struct.propTypes)
* ```
*/
function resolveUnstruct(ctx: ResolutionCtx, unstruct: Unstruct) {
const id = ctx.getUniqueName(unstruct);
const id = ctx.makeUniqueIdentifier(getName(unstruct), 'global');

ctx.addDeclaration(`\
struct ${id} {
Expand Down
4 changes: 2 additions & 2 deletions packages/typegpu/src/core/sampler/sampler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ export class TgpuLaidOutSamplerImpl<
}

[$resolve](ctx: ResolutionCtx): ResolvedSnippet {
const id = ctx.getUniqueName(this);
const id = ctx.makeUniqueIdentifier(getName(this), 'global');
const group = ctx.allocateLayoutEntry(this.#membership.layout);

ctx.addDeclaration(
Expand Down Expand Up @@ -186,7 +186,7 @@ class TgpuFixedSamplerImpl<T extends WgslSampler | WgslComparisonSampler>
}

[$resolve](ctx: ResolutionCtx): ResolvedSnippet {
const id = ctx.getUniqueName(this);
const id = ctx.makeUniqueIdentifier(getName(this), 'global');

const { group, binding } = ctx.allocateFixedEntry(
this.schema.type === 'sampler_comparison'
Expand Down
2 changes: 1 addition & 1 deletion packages/typegpu/src/core/texture/externalTexture.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ export class TgpuExternalTextureImpl implements TgpuExternalTexture, SelfResolva
}

[$resolve](ctx: ResolutionCtx): ResolvedSnippet {
const id = ctx.getUniqueName(this);
const id = ctx.makeUniqueIdentifier(getName(this), 'global');
const group = ctx.allocateLayoutEntry(this.#membership.layout);

ctx.addDeclaration(
Expand Down
4 changes: 2 additions & 2 deletions packages/typegpu/src/core/texture/texture.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -600,7 +600,7 @@ class TgpuFixedTextureViewImpl<T extends WgslTexture | WgslStorageTexture>
}

[$resolve](ctx: ResolutionCtx): ResolvedSnippet {
const id = ctx.getUniqueName(this);
const id = ctx.makeUniqueIdentifier(getName(this), 'global');
const { group, binding } = ctx.allocateFixedEntry(
isWgslStorageTexture(this.schema)
? {
Expand Down Expand Up @@ -642,7 +642,7 @@ export class TgpuLaidOutTextureViewImpl<T extends WgslTexture | WgslStorageTextu
}

[$resolve](ctx: ResolutionCtx): ResolvedSnippet {
const id = ctx.getUniqueName(this);
const id = ctx.makeUniqueIdentifier(getName(this), 'global');
const group = ctx.allocateLayoutEntry(this.#membership.layout);

ctx.addDeclaration(
Expand Down
2 changes: 1 addition & 1 deletion packages/typegpu/src/core/variable/tgpuVariable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ class TgpuVarImpl<TScope extends VariableScope, TDataType extends BaseData>
}

[$resolve](ctx: ResolutionCtx): ResolvedSnippet {
const id = ctx.getUniqueName(this);
const id = ctx.makeUniqueIdentifier(getName(this), 'global');
const pre = `var<${this.#scope}> ${id}: ${ctx.resolve(this.#dataType).value}`;

if (this.#initialValue) {
Expand Down
7 changes: 4 additions & 3 deletions packages/typegpu/src/data/autoStruct.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { createIoSchema } from '../core/function/ioSchema.ts';
import { isValidProp } from '../nameRegistry.ts';
import { validateProp } from '../nameUtils.ts';
import { getName, setName } from '../shared/meta.ts';
import { $internal, $repr, $resolve } from '../shared/symbols.ts';
import type { ResolutionCtx, SelfResolvable } from '../types.ts';
Expand Down Expand Up @@ -75,8 +75,9 @@ export class AutoStruct implements BaseData, SelfResolvable {
`Property name '${wgslKey}' causes naming clashes. Choose a different name.`,
);
}
if (!isValidProp(wgslKey)) {
throw new Error(`Property key '${key}' is a reserved WGSL word. Choose a different name.`);
const result = validateProp(wgslKey);
if (!result.success) {
throw new Error(`Invalid property key '${key}'${result.error ? `: ${result.error}` : ''}`);
}

this.#usedWgslKeys.add(wgslKey);
Expand Down
7 changes: 4 additions & 3 deletions packages/typegpu/src/data/struct.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { isValidProp } from '../nameRegistry.ts';
import { validateProp } from '../nameUtils.ts';
import { getName, setName } from '../shared/meta.ts';
import { $internal } from '../shared/symbols.ts';
import { schemaCallWrapper } from './schemaCallWrapper.ts';
Expand Down Expand Up @@ -40,8 +40,9 @@ export function INTERNAL_createStruct<TProps extends Record<string, BaseData>>(
isAbstruct: boolean,
): WgslStruct<TProps> {
Object.keys(props).forEach((key) => {
if (!isValidProp(key)) {
throw new Error(`Property key '${key}' is a reserved WGSL word. Choose a different name.`);
const result = validateProp(key);
if (!result.success) {
throw new Error(`Invalid property key '${key}'${result.error ? `: ${result.error}` : ''}`);
}
});

Expand Down
Loading
Loading