Skip to content

Commit

Permalink
feat: forbid instance and static class members with same name (#988)
Browse files Browse the repository at this point in the history 
### Summary of Changes

* Classes can no longer have instance and static members with the same
name. This was very confusing and frequently required special handling
(e.g. `.` vs. `#` separators for name paths in `{@link}` tags.

* `.` is now the only separator in name paths of `{@link}` tags,
regardless of whether it's an instance or static member.
  • Loading branch information
@lars-reimann
lars-reimann authored Apr 4, 2024
1 parent 79f9b08 commit 7fa6fd4
Show file tree
Hide file tree
Showing 5 changed files with 163 additions and 220 deletions.
37 changes: 16 additions & 21 deletions docs/language/common/comments.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ with no special meaning.

Documentation comments support [Markdown](https://www.markdownguide.org/) to format the text. Here is an example:

```sds
```sds hl_lines="2"
/**
* This is a documentation comment with **bold** and *italic* text.
*/
Expand All @@ -86,40 +86,35 @@ Documentation comments can contain tags to provide structured information.
`{@link}` is an **inline** tag that can be used to link to another declaration. It takes the name of the declaration as
an argument:

```sds
```sds hl_lines="2"
/**
* Computes the sum of two {@link Int}s.
*/
fun sum(a: Int, b: Int): sum: Int
```

To point to _static_ members of a [class][class] or an [enum variant][enum-variant] of an [enum][enum], write the name of
the containing declaration followed by a dot and the name of the member or enum variant:
To point to a member of a [class][class] or an [enum variant][enum-variant] of an [enum][enum], write the name of the
containing declaration followed by a dot and the name of the member or enum variant:

```sds
```sds hl_lines="2"
/**
* To create a Table, use {@link Table.fromCsv}.
* To create a Configuration, use {@link Configuration.fromFile}.
*/
class Table
```
class Configuration {
To point to an _instance_ member of a [class][class], write the name of the containing declaration followed by a hash and
the name of the member:

```sds
/**
* An alias for {@link List#size}.
*/
fun size(list: List<Any?>): size: Int
/**
* Creates a Configuration from a file.
*/
fun fromFile(file: String) -> result: Configuration
}
```


#### `@param`

Use `@param` to document a [parameter][parameter] of a callable declaration. This tag takes the name of the parameter
and its description as arguments. Since a callable can have multiple parameters, this tag can be used multiple times.

```sds
```sds hl_lines="4 5"
/**
* ...
*
Expand All @@ -134,7 +129,7 @@ fun sum(a: Int, b: Int): sum: Int
Use `@result` to document a [result][result] of a callable declaration. This tag takes the name of the result and its
description as arguments. Since a callable can have multiple results, this tag can be used multiple times.

```sds
```sds hl_lines="4"
/**
* ...
*
Expand All @@ -149,7 +144,7 @@ Use `@typeParam` to document a [type parameter][type-parameter] of a generic dec
type parameter and its description as arguments. Since a generic declaration can have multiple type parameters, this
tag can be used multiple times.

```sds
```sds hl_lines="4"
/**
* ...
*
Expand All @@ -163,7 +158,7 @@ class List<T>
The `@since` tag can be used to specify when a declaration was added. It takes the version as argument and should be
used only once.

```sds
```sds hl_lines="4"
/**
* ...
*
Expand Down
54 changes: 7 additions & 47 deletions packages/safe-ds-lang/src/language/documentation/safe-ds-documentation-provider.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import {
} from '../generated/ast.js';
import { isEmpty } from '../../helpers/collections.js';
import { SafeDsServices } from '../safe-ds-module.js';
import { getClassMembers, getEnumVariants, isStatic } from '../helpers/nodeProperties.js';
import { getClassMembers, getEnumVariants } from '../helpers/nodeProperties.js';

const PARAM_TAG = 'param';
const RESULT_TAG = 'result';
Expand Down Expand Up @@ -186,49 +186,22 @@ export class SafeDsDocumentationProvider extends JSDocDocumentationProvider {
}

private findTarget(node: AstNode, namePath: string): SdsDeclaration | undefined {
let [globalName, ...rest] = this.tokenizeNamepath(namePath);
// `rest` contains pairs of separators and names
if (!globalName || rest.length % 2 !== 0) {
let [globalName, ...rest] = namePath.split('.');
if (!globalName) {
/* c8 ignore next 2 */
return undefined;
}

let current = this.findGlobalDeclaration(node, globalName);

while (current && !isEmpty(rest)) {
const [separator, name] = rest.slice(0, 2);
rest = rest.slice(2);

if (separator === '.') {
current = this.findStaticMember(current, name);
} else if (separator === '#') {
current = this.findInstanceMember(current, name);
} else {
/* c8 ignore next 2 */
return undefined;
}
const name = rest.shift();
current = this.findMember(current, name);
}

return current;
}

private tokenizeNamepath(namePath: string): string[] {
const result = [];
let current = '';

for (const c of namePath) {
if (c === '.' || c === '#') {
result.push(current, c);
current = '';
} else {
current += c;
}
}

result.push(current);
return result;
}

private findGlobalDeclaration(node: AstNode, name: string): SdsDeclaration | undefined {
const description = this.findNameInPrecomputedScopes(node, name) ?? this.findNameInGlobalScope(node, name);
const target = description?.node;
Expand All @@ -240,33 +213,20 @@ export class SafeDsDocumentationProvider extends JSDocDocumentationProvider {
}
}

private findStaticMember(node: SdsDeclaration, name: string | undefined): SdsDeclaration | undefined {
private findMember(node: SdsDeclaration, name: string | undefined): SdsDeclaration | undefined {
if (!name) {
/* c8 ignore next 2 */
return undefined;
}

if (isSdsClass(node)) {
return getClassMembers(node).find((it) => isStatic(it) && it.name === name);
return getClassMembers(node).find((it) => it.name === name);
} else if (isSdsEnum(node)) {
return getEnumVariants(node).find((it) => it.name === name);
} else {
return undefined;
}
}

private findInstanceMember(node: SdsDeclaration, name: string | undefined): SdsDeclaration | undefined {
if (!name) {
/* c8 ignore next 2 */
return undefined;
}

if (isSdsClass(node)) {
return getClassMembers(node).find((it) => !isStatic(it) && it.name === name);
} else {
return undefined;
}
}
}

const isBlockTag = (element: JSDocElement): element is JSDocTag => {
Expand Down
7 changes: 1 addition & 6 deletions packages/safe-ds-lang/src/language/validation/names.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ import {
getParameters,
getResults,
getTypeParameters,
isStatic,
streamBlockLambdaResults,
streamPlaceholders,
} from '../helpers/nodeProperties.js';
Expand Down Expand Up @@ -224,11 +223,7 @@ export const classMustContainUniqueNames = (node: SdsClass, accept: ValidationAc
accept,
);

const instanceMembers = getClassMembers(node).filter((it) => !isStatic(it));
namesMustBeUnique(instanceMembers, (name) => `An instance member with name '${name}' exists already.`, accept);

const staticMembers = getClassMembers(node).filter(isStatic);
namesMustBeUnique(staticMembers, (name) => `A static member with name '${name}' exists already.`, accept);
namesMustBeUnique(getClassMembers(node), (name) => `A class member with name '${name}' exists already.`, accept);
};

export const enumMustContainUniqueNames = (node: SdsEnum, accept: ValidationAcceptor): void => {
Expand Down
23 changes: 6 additions & 17 deletions packages/safe-ds-lang/tests/language/documentation/safe-ds-documentation-provider.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,7 @@ describe('SafeDsDocumentationProvider', () => {
testName: 'link (instance attribute)',
code: `
/**
* {@link MyClass#myAttribute}
* {@link MyClass.myAttribute}
*/
fun myFunction1()
Expand All @@ -369,7 +369,7 @@ describe('SafeDsDocumentationProvider', () => {
}
`,
predicate: isSdsFunction,
expectedDocumentation: `[MyClass#myAttribute](`,
expectedDocumentation: `[MyClass.myAttribute](`,
},
{
testName: 'link (static attribute)',
Expand All @@ -390,7 +390,7 @@ describe('SafeDsDocumentationProvider', () => {
testName: 'link (instance method)',
code: `
/**
* {@link MyClass#myMethod}
* {@link MyClass.myMethod}
*/
fun myFunction1()
Expand All @@ -399,7 +399,7 @@ describe('SafeDsDocumentationProvider', () => {
}
`,
predicate: isSdsFunction,
expectedDocumentation: `[MyClass#myMethod](`,
expectedDocumentation: `[MyClass.myMethod](`,
},
{
testName: 'link (nested class)',
Expand Down Expand Up @@ -450,7 +450,7 @@ describe('SafeDsDocumentationProvider', () => {
testName: 'link (long chain)',
code: `
/**
* {@link MyClass.NestedClass#myMethod}
* {@link MyClass.NestedClass.myMethod}
*/
fun myFunction1()
Expand All @@ -461,7 +461,7 @@ describe('SafeDsDocumentationProvider', () => {
}
`,
predicate: isSdsFunction,
expectedDocumentation: `[MyClass.NestedClass#myMethod](`,
expectedDocumentation: `[MyClass.NestedClass.myMethod](`,
},
{
testName: 'link (unresolved global)',
Expand All @@ -474,17 +474,6 @@ describe('SafeDsDocumentationProvider', () => {
predicate: isSdsFunction,
expectedDocumentation: `myFunction2`,
},
{
testName: 'link (wrong container for instance)',
code: `
/**
* {@link myFunction1#test}
*/
fun myFunction1()
`,
predicate: isSdsFunction,
expectedDocumentation: `myFunction1#test`,
},
{
testName: 'link (wrong container for static)',
code: `
Expand Down
Loading

0 comments on commit 7fa6fd4

Please sign in to comment.