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

x/mobile: Exported structs not retaining field values in Swift #32008

Open
triztian opened this issue May 13, 2019 · 6 comments

Comments

@triztian
Copy link

commented May 13, 2019

What version of Go are you using (go version)?

$ go version go1.12.4 darwin/amd64

Does this issue reproduce with the latest release?

Haven't tested

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOBIN="/Users/tristian/go/bin"
GOCACHE="/Users/tristian/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/tristian/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/Cellar/go/1.12.4/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.12.4/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/g1/v3hz1sm92nv7wbzpg3dx_h_h0000gp/T/go-build064001605=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I'm using the latest gomobile, installed from this commit:

commit 32b2708ab17190067486adc3513cae8dc2a7e5a4 (HEAD -> master, origin/master, origin/HEAD)
Author: Mark Villacampa <m@markvillacampa.com>
Date:   Thu May 9 10:20:13 2019 +0000

In my go source, I added a struct type that is exposed as part of Gomobile:

type Config struct {
  OSName string
  OSDevice string
}

// NewConfig()
func NewConfig() *Config { 
  return &Config{}
}

In the Swift code I use the following to create an instance and set some fields.

// "Mobile" is the prefix package name
// var config = MobileNewConfig() (tried this too)
let config = MobileNewConfig()

config?.osName = "some name"
config?.osDevice = "some string"

I build the framework like so:

GO111MODULE=off gomobile bind -a -v -target=ios/arm,ios/arm64,ios/amd64 example.com/mobile

What did you expect to see?

I expected to see the config variable retain the assigned string values in the swift code.

What did you see instead?

The config variable not retaining the swift values, for example take a look at this debugger capture:

Debugging

@gopherbot gopherbot added this to the Unreleased milestone May 13, 2019

@gopherbot gopherbot added the mobile label May 13, 2019

@triztian

This comment has been minimized.

Copy link
Author

commented May 14, 2019

I've created the following repo to reproduce the issue:

@triztian

This comment has been minimized.

Copy link
Author

commented Jul 20, 2019

I think I've found the issue, essentialy when binding the framework with the following command:

gomobile bind -v -target=ios -work .

If we cd into the generated objc files we can see the following under src/gobind/Mobile_darwin.m

- (NSString* _Nonnull)osName {
	int32_t refnum = go_seq_go_to_refnum(self._ref);
	nstring r0 = proxymobile_Config_OSName_Get(refnum);
	NSString *_r0 = go_seq_to_objc_string(r0);
	return _r0;
}

- (void)setOSName:(NSString* _Nonnull)v {
	int32_t refnum = go_seq_go_to_refnum(self._ref);
	nstring _v = go_seq_from_objc_string(v);
	proxymobile_Config_OSName_Set(refnum, _v);
}

Whereas the Label property accessors look like this:

- (NSString* _Nonnull)label {
	int32_t refnum = go_seq_go_to_refnum(self._ref);
	nstring r0 = proxymobile_Config_Label_Get(refnum);
	NSString *_r0 = go_seq_to_objc_string(r0);
	return _r0;
}

- (void)setLabel:(NSString* _Nonnull)v {
	int32_t refnum = go_seq_go_to_refnum(self._ref);
	nstring _v = go_seq_from_objc_string(v);
	proxymobile_Config_Label_Set(refnum, _v);
}
@triztian

This comment has been minimized.

Copy link
Author

commented Jul 20, 2019

I've updated the repo that reproduces this issue:

If the following patch is applied to gomobile:

golang/mobile@master...triztian:issue/32008

then we reinitialize: gomobile init and then lastly re-run the following in the repro issue:

make clean run

The executable behaves as expected, i.e. I get the following:

 λ ~/go/src/github.com/triztian/gomobileprops (master): make clean run
rm -rf Mobile.framework main
gomobilex bind -v -target=ios -work .
WORK=/var/folders/g1/v3hz1sm92nv7wbzpg3dx_h_h0000gp/T/gomobile-work-227841999/go-build475590256
runtime/cgo
golang.org/x/mobile/internal/mobileinit
gobind
WORK=/var/folders/g1/v3hz1sm92nv7wbzpg3dx_h_h0000gp/T/gomobile-work-227841999/go-build084453767
runtime/cgo
golang.org/x/mobile/internal/mobileinit
gobind
WORK=/var/folders/g1/v3hz1sm92nv7wbzpg3dx_h_h0000gp/T/gomobile-work-227841999/go-build110091045
runtime/cgo
golang.org/x/mobile/internal/mobileinit
gobind
WORK=/var/folders/g1/v3hz1sm92nv7wbzpg3dx_h_h0000gp/T/gomobile-work-227841999/go-build083494747
runtime/cgo
golang.org/x/mobile/internal/mobileinit
gobind
write Mobile.framework/Versions/A/Headers/Mobile.objc.h
write Mobile.framework/Versions/A/Headers/Universe.objc.h
write Mobile.framework/Versions/A/Headers/ref.h
write Mobile.framework/Versions/A/Headers/Mobile.h
write Mobile.framework/Resources/Info.plist
write Mobile.framework/Versions/A/Modules/module.modulemap
WORK=/var/folders/g1/v3hz1sm92nv7wbzpg3dx_h_h0000gp/T/gomobile-work-227841999
swiftc -g -F. main.swift
<module-includes>:2:9: note: in file included from <module-includes>:2:
#import "Headers/Mobile.objc.h"
        ^
/Users/tristian/go/src/github.com/triztian/gomobileprops/./Mobile.framework/Headers/Mobile.objc.h:21:1: warning: conflicting nullability specifier on return types, 'nullable' conflicts with existing specifier 'nonnull'
- (nullable instancetype)init;
^
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/usr/include/objc/NSObject.h:63:1: note: previous declaration is here
- (instancetype)init
^
./main
Success osName: THE OSNAME

The generated accessors after applying the patch are the following:

- (NSString* _Nonnull)label {
	int32_t refnum = go_seq_go_to_refnum(self._ref);
	nstring r0 = proxymobile_Config_Label_Get(refnum);
	NSString *_r0 = go_seq_to_objc_string(r0);
	return _r0;
}

- (void)setLabel:(NSString* _Nonnull)v {
	int32_t refnum = go_seq_go_to_refnum(self._ref);
	nstring _v = go_seq_from_objc_string(v);
	proxymobile_Config_Label_Set(refnum, _v);
}

- (NSString* _Nonnull)osName {
	int32_t refnum = go_seq_go_to_refnum(self._ref);
	nstring r0 = proxymobile_Config_OSName_Get(refnum);
	NSString *_r0 = go_seq_to_objc_string(r0);
	return _r0;
}

- (void)setOsName:(NSString* _Nonnull)v {
	int32_t refnum = go_seq_go_to_refnum(self._ref);
	nstring _v = go_seq_from_objc_string(v);
	proxymobile_Config_OSName_Set(refnum, _v);
}

I'd create a PR but if I run the go test ./bind command many of the tests fail against the golden files, for example ./bind/testdata/doc.objc.m.golden expects the following for a field named sf:

- (NSString* _Nonnull)sf {
	int32_t refnum = go_seq_go_to_refnum(self._ref);
	nstring r0 = proxydoc_S_SF_Get(refnum);
	NSString *_r0 = go_seq_to_objc_string(r0);
	return _r0;
}

- (void)setSF:(NSString* _Nonnull)v {
	int32_t refnum = go_seq_go_to_refnum(self._ref);
	nstring _v = go_seq_from_objc_string(v);
	proxydoc_S_SF_Set(refnum, _v);
}

I found the capitalization rules here:

@eliasnaur what should I do to create an PR that is acceptable, is there a specific process to update the golden files?

I think this will affect anyone that follows the naming guidelines for acronyms and is an issue that manifests silently.

@eliasnaur

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2019

You can update the golden files with

go test ./bind -update

@triztian

This comment has been minimized.

Copy link
Author

commented Jul 20, 2019

Thank you for the quick reply, I've done that and it seems to have fixed the tests:

 λ ~/go/src/golang.org/x/mobile (issue/32008b): go test ./bind/ -update
ok  	golang.org/x/mobile/bind	15.632s
 λ ~/go/src/golang.org/x/mobile (issue/32008b): go test -v ./bind/
=== RUN   TestGenObjc
--- PASS: TestGenObjc (0.12s)
=== RUN   TestGenJava
--- PASS: TestGenJava (6.97s)
=== RUN   TestGenGo
--- PASS: TestGenGo (0.06s)
=== RUN   TestGenGoJavaWrappers
--- PASS: TestGenGoJavaWrappers (6.84s)
=== RUN   TestGenGoObjcWrappers
--- PASS: TestGenGoObjcWrappers (1.71s)
=== RUN   TestCustomPrefix
--- PASS: TestCustomPrefix (0.05s)
=== RUN   TestLowerFirst
--- PASS: TestLowerFirst (0.00s)
=== RUN   TestCapitalize
--- PASS: TestCapitalize (0.00s)
=== RUN   TestSelectorExprTypeName
--- PASS: TestSelectorExprTypeName (0.00s)
PASS
ok  	golang.org/x/mobile/bind	(cached)

I've submitted a PR here:

triztian added a commit to triztian/mobile that referenced this issue Jul 20, 2019
bind: Fix setter name generation for ObjC
The generation for getters and setters of Go
exported fields that followed the accroynyms
rule, i.e. for a field named `OSName` the
resulting getter and settere were `osName` and
`setOSName` respectively, however the Obj-C rules
defined here:

  * https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/EncapsulatingData/EncapsulatingData.html

expected for the names to be the following: `osName`
and `setOsName`.

This commit updates setter name generation by capitalizing
the field name and leaves the getter name unchaged, this
also updates the golden files.

Fixes golang/go#32008
triztian added a commit to triztian/mobile that referenced this issue Aug 2, 2019
bind: Fix setter name generation for ObjC
The generation for getters and setters of Go
exported fields that followed the accroynyms
rule, i.e. for a field named `OSName` the
resulting getter and settere were `osName` and
`setOSName` respectively, however the Obj-C rules
defined here:

  * https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/EncapsulatingData/EncapsulatingData.html

expected for the names to be the following: `osName`
and `setOsName`.

This commit updates setter name generation by capitalizing
the field name and leaves the getter name unchaged, this
also updates the golden files.

Fix golang/go#32008
@gopherbot

This comment has been minimized.

Copy link

commented Aug 2, 2019

Change https://golang.org/cl/186978 mentions this issue: bind: Fix setter name generation for ObjC

triztian added a commit to triztian/mobile that referenced this issue Aug 2, 2019
bind: Fix setter name generation for ObjC
The generation for getters and setters of Go
exported fields that followed the acronyms
rule, i.e. for a field named `OSName` the
resulting getter and setter were `osName` and
`setOSName` respectively, however the Obj-C rules
defined here:

  * https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/EncapsulatingData/EncapsulatingData.html

expected for the names to be the following: `osName`
and `setOsName`.

This commit updates setter name generation by capitalizing
the first char and lower casing the second one, it leaves
the getter name unchaged, this also updates the golden files.

Fix golang/go#32008
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.