Skip to content
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.

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

Binary Signature Name proposal #303

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Binary Signature Name proposal #303

wants to merge 3 commits into from

Conversation

elizarov
Copy link
Contributor

@elizarov elizarov commented Jun 2, 2022

Please, use this PR for minor corrections (like typos and text style) and use KEEP-302 for the discussion on the substance of the proposal.

@elizarov elizarov mentioned this pull request Jun 2, 2022
Copy link
Contributor

@lounres lounres left a comment

Choose a reason for hiding this comment

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

I found several typos.

proposals/multiplatform/binary-signature.md Outdated Show resolved Hide resolved
Comment on lines +430 to +432
Objective-C has relaxed rules on overrides. In particular, it is possible to change the parameter types in Object-C on override.
These relaxed checks are implemented only in Kotlin/Native for Kotlin code that is produced as a result of ObjC-interop tooling and
their actual specifications are out of the scope of this KEEP.
Copy link
Contributor

Choose a reason for hiding this comment

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

Objective-C is called "Objective-C" (the 1st sentence), "Object-C" (the 1st sentence) and "ObjC" (the 2nd sentence) at the same time. Is it a typo or intended?

Copy link
Contributor Author

@elizarov elizarov Jun 6, 2022

Choose a reason for hiding this comment

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

Object-C is a typo, while an ObjC is a proper abbreviation.


In general, it is not in Kotlin style to have functions that different only in parameter name like it is customary
in Objective-C and Swift, for example. In Kotlin you'd give these functions different names. However, it is conceivable
that some domain-specific DSLs might want to have this kind of feature. The following code does not compile:
Copy link
Contributor

Choose a reason for hiding this comment

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

There is phrase "domain-specific DSLs" (the 3rd sentence) which is equal to "domain-specific domain-specific languages". Is the doubling a typo or intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Typo. Should be just DSLs.

}

class Derived : Base() {
@SingatureName("foo")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@SingatureName("foo")
@BinarySignatureName("foo")

}

class Derived : Base() {
@SingatureName("bar")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@SingatureName("bar")
@BinarySignatureName("bar")

Comment on lines +194 to +195
Normally, such overloads, that are different only in parameter names, are not allowed in Kotlin, so a special
Objective-C specific rules are currently used for Objective-C interoperability. However, these Objective-C-specific
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Normally, such overloads, that are different only in parameter names, are not allowed in Kotlin, so a special
Objective-C specific rules are currently used for Objective-C interoperability. However, these Objective-C-specific
Normally, such overloads, that are different only in parameter names, are not allowed in Kotlin, so special
Objective-C specific rules are currently used for Objective-C interoperability. However, these Objective-C-specific

An indefinite article is almost never used with plurals, it can be dropped

Kotlin uses different signatures in different stages of compilation process:

* [Call resolution signature](#call-resolution-signature)
* [Override matchine signature](#override-matching-signature)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* [Override matchine signature](#override-matching-signature)
* [Override matching signature](#override-matching-signature)

The change of parameter names is binary compatible in Kotlin, but not, in general, source compatible.
* It includes a return type. That makes it different from the call resolution signature, too.
While the change of the return type to a more specific type can be mostly source compatible, it is not a binary compatible change.
* It use the name specified in `@BinarySignatureName`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* It use the name specified in `@BinarySignatureName`,
* It uses the name specified in `@BinarySignatureName`,

#### Override matching check

Declarations are grouped by BOTH a source name and a signature name to detect conflicting
overloads using the current Kotlin rules. That is, in each, group pairs of declarations are checked w.r.t. their
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
overloads using the current Kotlin rules. That is, in each, group pairs of declarations are checked w.r.t. their
overloads using the current Kotlin rules. That is, in each group, pairs of declarations are checked w.r.t. their

The specified signature name affects linking of the resulting binary and thus has the following platform-specific effects:

* **Kotlin/JVM**: uses JAR file format as the binary artifact produced by the compiler, so the signature name is used as
a default value of `@JvmName` to ensure that it has the desired effect on the resulting binary signature.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
a default value of `@JvmName` to ensure that it has the desired effect on the resulting binary signature.
a default value of `@JvmName` to ensure that it has the desired effect on the resulting binary signature.

In Markdown, double space at line end forces a hard line break

The Kotlin standard library uses `@SinceKotlin("...")` and `@DeprecatedSinceKotlin(hiddenSince = "...")` to hide
declarations conditionally, based on the current API version in use (the value of the `-api-version` argument
when compiling the module where the usage is located). For the purposes of **call resolution** check,
they will taken into account, too:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
they will taken into account, too:
they are taken into account, too:

they will taken into account, too:

* Declarations with `@SinceKotlin(version)`, when the current API version is below the specified version, are excluded from the check.
* Declarations with `@DeprecatedSinceKotlin(hiddenSince = version)`, when the current API version is the same or more recent as the version, are excluded from the check.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Declarations with `@DeprecatedSinceKotlin(hiddenSince = version)`, when the current API version is the same or more recent as the version, are excluded from the check.
* Declarations with `@DeprecatedSinceKotlin(hiddenSince = version)`, when the current API version is the same or more recent as the specified version, are excluded from the check.

fun addSection(footer: String) { /* ... */ }
```

> In fact, only them is required to have `@BinarySginatureName` name annotation to compile this code.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
> In fact, only them is required to have `@BinarySginatureName` name annotation to compile this code.
> In fact, only one of them is required to have `@BinarySginatureName` name annotation to compile this code.

```

The `@BinarySignatureName` annotation affects the resolution algorithm in the compiler on a deep level, so this annotation,
by itself, has to be resolved before everything else. Hence, there is an additional restrictions on usage of this annotation:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
by itself, has to be resolved before everything else. Hence, there is an additional restrictions on usage of this annotation:
by itself, has to be resolved before everything else. Hence, there are additional restrictions on usage of this annotation:

a great deal of type information that is otherwise available in Kotlin. It means, that a valid change during separate
compilation of different libraries can produce a pair of libraries that link together successfully, yet are not
consistent from the Kotlin's type-system point of view. Trying to build those after linking into a platform binary
would either crash the compiler or produce a broken executable. To avoid that, we would need to specify an implement
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
would either crash the compiler or produce a broken executable. To avoid that, we would need to specify an implement
would either crash the compiler or produce a broken executable. To avoid that, we would need to specify and implement

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants