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

Regression when calling macros and using haxe.io.Bytes #6543

Closed
hklindworth opened this issue Aug 30, 2017 · 16 comments
Closed

Regression when calling macros and using haxe.io.Bytes #6543

hklindworth opened this issue Aug 30, 2017 · 16 comments
Assignees

Comments

@hklindworth
Copy link

I encountered a regression that is most likely related to the new macro handling introduced with Haxe 4. I can reproduce it when calling a macro function on a class which uses haxe.io.Bytes. With Haxe3 compilation is possible.

This is a simple OpenFL project that triggers the wrong behavior and compiler fails:
https://github.com/hklindworth/eval-issue-openfl

openfl build html5
/Dev/haxe/lime/lime/utils/Bytes.hx:23: characters 10-43 : haxe.io.Bytes does not have a constructor
/Dev/haxe/openfl/openfl/utils/ByteArray.hx:286: characters 3-26 : haxe.io.Bytes does not have a constructor
/Dev/haxe/openfl/openfl/utils/ByteArray.hx:786: characters 3-4 : Unknown identifier : b
Field index for length not found on prototype haxe.io.Bytes

This is a minimal project that show also some error. No internal compiler error, but constructor cannot be accessed, even though it is made available via @:access:
https://github.com/hklindworth/eval-issue

haxe release.hxml 
Main.hx:22: characters 10-43 : haxe.io.Bytes does not have a constructor

I also tried to add the private constructor to

extern class Bytes {

This fixes the "does not have a constructor" error message, but not the compiler crash.

@Simn
Copy link
Member

Simn commented Aug 30, 2017

Yes, haxe.io.Bytes does in fact not have a constructor on this target. It might be possible to add it for backwards compatibility, but I'm not sure if that's worth it. I think the best solution is to use the public API (Bytes.ofData) instead of trying to access implementation details...

@hklindworth
Copy link
Author

hklindworth commented Aug 30, 2017

Currently lime is accessing the private constructor:
https://github.com/openfl/lime/blob/9914b2498fc01814a45dffb5fc30e88f781ed267/lime/utils/Bytes.hx#L18
Also openfl is accessing private members. These can be fixed there. But it still seems strange to me that these errors only occour when you call a macro function.

And even when I fix these issues (just commented them out), the compilation still fails with
Field index for length not found on prototype haxe.io.Bytes
This is the most problematic issue, especially since no filename or linenumber is given that is hinting at the problem.

@hklindworth
Copy link
Author

PS: I just created another test project which is selfcontaining (unfortuntely quite big) and triggers a compiler assertion when you call the macro function and compiles successfully when not calling the macro function:

https://github.com/hklindworth/eval-issue-lime

Hope this helps

@Simn Simn self-assigned this Aug 30, 2017
@Simn
Copy link
Member

Simn commented Aug 30, 2017

I'll take a look at the assertion, that clearly shouldn't happen. Any idea how to reproduce the Field index for length not found on prototype haxe.io.Bytes error?

As for the constructor call, that should really be fixed in lime and it shouldn't access any private members in general.

@Simn
Copy link
Member

Simn commented Aug 30, 2017

The assertion comes from this code: https://github.com/hklindworth/eval-issue-lime/blob/master/lime/lime/app/Application.hx#L94

There's probably an assumption that a var expression is not the only expression in a block, as that is rather pointless. Still, it obviously shouldn't cause an assertion failure.

@Simn
Copy link
Member

Simn commented Aug 30, 2017

Fixed the var issue, and added that constructor after all because I guess it doesn't hurt.

Still curious about that length issue though!

@hklindworth
Copy link
Author

I adjusted the test project:
https://github.com/hklindworth/eval-issue-openfl

When you compile it now via haxe release.hxml is fails with
Field index for length not found on prototype haxe.io.Bytes

With Haxe3 it fails with
openfl/utils/ByteArray.hx:278: lines 278-319 : Missing super constructor call lime/lime/utils/Bytes.hx:67: characters 16-41 : Unknown<0> cannot be constructed lime/lime/utils/AssetLibrary.hx:659: characters 17-43 : Unknown<0> cannot be constructed openfl/utils/ByteArray.hx:278: lines 278-319 : Missing super constructor call lime/lime/media/AudioSource.hx:18: characters 25-49 : Unknown<0> cannot be constructed

@Simn
Copy link
Member

Simn commented Aug 30, 2017

That just gives me Type not found : ApplicationMain

@hklindworth
Copy link
Author

hklindworth commented Aug 30, 2017

That seems to a bug in my test example. Just pushed a fix.

On my machine it is now:
hendrik:eval-issue-openfl hendrik$ haxe release.hxml openfl/utils/ByteArray.hx:278: lines 278-319 : Missing super constructor call Field index for length not found on prototype haxe.io.Bytes

If you do not have the Field index length message, this seems like a good sign to me. I will recheck my examples ones the new nightly build of haxe has been released.

Thank you very much for your great help!

@Simn
Copy link
Member

Simn commented Aug 30, 2017

I changed it so the error message gives a position now: openfl/utils/ByteArray.hx:324: characters 3-9 : Field index for length not found on prototype haxe.io.Bytes. The line in question tries to set the length of a Bytes object.

@Simn
Copy link
Member

Simn commented Aug 31, 2017

Try now. If it complains about some b field, just use the object itself instead of the field (obj instead of obj.b).

@hklindworth
Copy link
Author

hklindworth commented Aug 31, 2017

Thanks, the compiler behavior greatly improved. No assertions and no compiler stops anymore.

Openfl still has one access to the private variable b that is not easy to remove
https://github.com/openfl/openfl/blob/31f6f6437ea37652b3fe69dea393c88f8a79b8b1/openfl/utils/ByteArray.hx#L814

Question is, if this should rather be fixed in openfl or in Haxe4? I still find a bit confusing that this error only appears when using macros in Main.hx. When you uncomment following line, the project compiles:
https://github.com/hklindworth/eval-issue-openfl/blob/180ba3d7836b1de7303d26e020a50e72255aa804/Main.hx#L29

I could try to change OpenFL ByteArrayData not to extend haxe.io.Bytes but to embed it.

@Simn
Copy link
Member

Simn commented Sep 1, 2017

The correct usage is b.getData(), i.e. the public API.

@hklindworth
Copy link
Author

Alright, I agree that it is better to fix openfl.

@jgranick
Copy link

jgranick commented Sep 2, 2017

I'm trying to understand this, the issue is that b does not exist in the macro context? Can we just #if !macro around any references to the b object, or are there other targets that do not have a b property?

Are there any recommendations for being able to test Haxe 4 development builds alongside using Haxe 3 stable builds on the same system?

@hklindworth
Copy link
Author

One way to test Haxe3 / Haxe4 at the same time (on MacOS):

  1. copy existing /usr/local/lib/haxe to /usr/local/lib/haxe3. This way all the haxelibs are initially the same between Haxe3 & Haxe4
  2. overwrite all files in /usr/local/lib/haxe with the latest nightly build from http://hxbuilds.s3-website-us-east-1.amazonaws.com/builds/haxe/mac/
  3. Now you can just rename the directories to switch between Haxe3 and Haxe4. I wrote a small shell-script to switch between versions:
#!/bin/sh

if [ -d /usr/local/lib/haxe4 ]; then
	mv /usr/local/lib/haxe /usr/local/lib/haxe3
	mv /usr/local/lib/haxe4 /usr/local/lib/haxe
else
	mv /usr/local/lib/haxe /usr/local/lib/haxe4
	mv /usr/local/lib/haxe3 /usr/local/lib/haxe
fi

Simn added a commit that referenced this issue Sep 12, 2017
* Actually disable the tests

* [java/cs] Make sure that cast new NativeArray uses the right type parameters

Closes #5751

* Fix test

* [cs] Only generate casts on NativeArray if the type parameters are not the same

Fixes fast_cast define

* Fix #6375 (#6376)

* also check tempvar'd this field access when typing getter/setter (closes #6375)

* add test

* [java/cs] Make sure to collect the right type parameters on anonymous functions

Closes #5793

* [neko] fix Sys.programPath() in nekotools booted exe (close #5708)

* don't remove enumIndex optimizations because EnumFlags is super important

* [TravisCI] fix boolean logic for neko installation

* [TravisCI] Fix neko installation on OSX

* Treat object array sets as having gc references

* [appveyor] merge php tests with the others

* [php7] fix haxe.io.Input.readAll() with disabled analyzer optimizations (closes #6387)

* changelog

* [php7] fix accessing static methods of String class stored into a variable (#4667)

* [php7] replace `Syntax.binop(.=)` with more analyzer-friendly `= Syntax.binop(.)`

* fixed stacks not being correctly reset in case of not unify detection (close #6235)

* add native mysqli error message when throwing an exception (#6390)

* use one-based error format, add -D old-error-format to keep zero-based behavior (#6391)

* [lua] old string metatable index is a function, not a table. fixes #6377

* [lua] previous string __index can be a table or a function

* fix position tests (see #6391)

* fix import.hx diagnostics condition (closes #6382)

* deal with EDisplay in assignment patterns (closes #6381)

* small toplevel collector cleanup (is going to help with #6341)

* filter duplicate toplevel identifiers (closes #6341)

* [eval] catch Failure on all Process calls (closes #6333)

* [eval] implement missing Date.fromString formats (closes #6323)

* print usage to stdout instead of stderr (closes #6363)

* [make] try to fix build directory dependency problem

* [lexer] count braces in code strings (closes #6369)

* [dce] keep everything if a class extends an extern (closes #6168)

* [display] delay metadata check behind field typing (closes #6162)

* fix test

* [matcher] fix assignment pattern display differently (closes #6395)

* fix json range column positions (back to zero-based after nicolas change)

gotta look into eval position stuff and check there too

* [java] Initial support for treating Null<BasicType> as their actual Java counterparts

* [java] java.lang.Long is also a boxed type

* [java] Properly rewrite basic type parameters as their boxed counterparts

Closes #784
Related #3728

* [java/cs] Add test

Closes #2688

* [cs] Automatically set nativegen to structs, and enforce that they cannot be subclassed, nor contain a constructor with no arguments

Closes #5399

* [java/cs] Make sure to reset old values when resizing a map

Closes #5862

* [java/cs] Avoid boxing with Runtime.compare expressions on dynamic field access

Closes #4047

* [java/cs] Avoid boxing when comparing Null<> basic types

Closes #4048

* [cs] Add C# optional arguments on Haxe-defined functions that have optional arguments

Closes #4147

* [java/cs] Prepare the code for the abstract null type PR

* [java/cs] Cast field field accesses to type parameters to their implementation instead of going through reflection

Closes #3790

* [cpp] No FastIterator for cppia

* [TravisCI] fix "Please check that the PPA name or format is correct."

* add test (closes #6215)

* [hl] disable failing test

* [java] disable failing test

* [parser] don't make reification call positions span the entire expression (closes #6396)

* Revert "[cs] Add C# optional arguments on Haxe-defined functions that have optional arguments"

This reverts commit 261e73c.

* don't use global status var for newly created anon types (closes #6400)

* [lua] fix lazy function type handling for anon method binding (#6368)

* Make _build source directory on demand

* [Makefile] always generate version.ml

such that the version info is always up-to-date

* ignore haxelib run.n changes

* [macro] add haxe.macro.Expr.TypeDefinition.doc field and support printing it with haxe.macro.Printer (#6401)

* Change Null<T> to an abstract (#6380)

* use abstract instead of typedef for Null<T>

* make Null<T> a @:coreType (broken on C#/Java)

* fix infinite recursion

* fix flash

* fix overloads

* Do not dce fields which override extern fields (discussion in 23f94bf) (#6168)

* [eval] fix inconsistent error formatting (#6410)

see vshaxe/vshaxe#138

* [cs/java] add an expr filter that optimizes Type.createEmptyInstace when the type is known

this helps avoid redundant casting overhead on c# when type parameters are involved (e.g. `(Type.createInstance(C) : C<Int>)` becomes `new C<int>(EMPTY)`

* Changed is_extern_field to is_physical_field (#6404)

* changed is_extern_field to is_physical_field

* fix is_extern_field->is_physical_field change in genlua

* [js] reserve flattened paths for variable renaming (closes #6409)

* [inliner] only unify_min if we go from no-type to some type (closes #6406)

* [display] don't show Module.platform.hx in import completion (closes #6408)

* [java] disable failing test

* fix comments in php7.Syntax

* [macro] flush pass in typing-timer

Also make `cast_of_unify` go through typing-timer. Closes #6402

* [parser] fix binop position spanning (closes #6399)

* [display] respect `@:noCompletion` for toplevel (closes #6407)

* Use Meta.Pos instead of Meta.Custom ":pos" (#6419)

That way, it shows up in --help-metas.

* [parser] fix more positions (closes #6416) (closes #6417)

* [display] don't show non-static abstract fields via using (closes #6421)

* [typer] disallow accessing non-static abstract field statically (closes #5940)

* [display] do not generate property accessor calls if we are displaying the property (closes #6422)

* TIdent (#6424)

* add TIdent

* remove Meta.Unbound and break everything

* first replacement batch

* fix neko/hl

* fix java/cs

* fix cpp/php and maybe lua

* fix cppia and maybe fix lua (again)

* remove unused NoPatternMatching define

* support specifying platforms for defines (so they are mentioned in --help-defines)

* [dce] don't add @:directlyUsed to a class if it's used from within its methods (fixes #6425)

* add some platform tags to define info

* [gencommon] remove unused fix_tparam_access argument

* [lua] introduce @:luaDotMethod for using dot-style invocation on appropriate extern instances

* [cpp] Split CppGlobal to allow untyped identifiers as externs. Type the __trace function more strongly

* [cpp] Do not use GC memory for const enums

* support argument names in function type (closes #4799) (#6428)

* [lua] fix inconsistent function assignment (#6426)

* Revert "support argument names in function type (closes #4799) (#6428)"

This reverts commit b225e9f.

* [php7] implemented Lib.mail()

* [php7] php.Lib.rethrow()

* [php7] Global.require(), Global.include()

* [php7] implemented php.Lib.loadLib()

* [php7] implemented php.Lib.getClasses() (closes #6384)

* [lua] generate prototypes with plain field access, rather than through _hx_a(...)

* [lua] drop unused param

* [lua] ocp-indent

* [cpp] Use stack allocations to get class vtables

* [cpp] No need to mark class statics if there are none

* [cpp] Fix hasMarkFunc condition

* [lua] fix multireturn return type

* [TravisCI] move osx_image to matrix

* [eval] catch a Break (closes #6443)

* [eval] support extending special classes (closes #6446)

* [display] check field display on extern function fields (closes #6442)

* [typer] don't lose return expressions even if they error (closes #6445)

* fix test

* make -D old-error-format deal with display completion being sent in bytes instead of chars

* [js] apply var escaping rules to function arguments as well (fixes #6449)

* [TravisCI] use brew bundle to install brew formulae

It avoids getting "xxx already installed" error.

* [lua] use _hx_string_wrapper helper function instead of modifying string prototype

* [lua] wrap string operations so that metatable changes are not necessary

* (HL) Fixed non-blocking SSL Socket connect (#6452)

* added getThis() in jquery event, allow to easily get real this in a callback

* [lua] faster std Math min/max (#6455)

ternary expressions seem to be faster when not part of an inline

* Add jit option to cppia host

* Add alias 'Single' for cpp.Float32.  Closes HaxeFoundation/hxcpp#555

* Check for null object in Reflect.callMethod.  Closes HaxeFoundation/hxcpp#593

* [js] consider @:native type names when collecting reserved names for local vars (see #6448)

* [js] support unqualified static field access for @:native("") classes (see #6448)

* [js] consider statics of @:native("") classes when renaming local vars (closes #6448)

* Disable extra cppia test until I can debug

* oops, add #if

* [hl] fixed Type.getClass with interface/virtual input

* [js] Add compiler switch to generate enums as objects instead of arrays (#6350)

* add -D js_enums_as_objects switch

* add $hxEnums, change __enum__ to String

* whitespace

* resolve genjs.ml conflict

* add -D js-enums-as-objects to tests

* [lua] fix/simplify Rex extern

* [lua] one more Rex tweak

* fix haxe.web.Dispatch

* [cpp] Remove some scalar casts when not requires, and add some to reduce warnings.  Closes HaxeFoundation/hxcpp#485

* [jQuery] regenerate extern with added Event.getThis()

andyli/jQueryExternForHaxe@4a6e3d5

* [TravisCI] install a patched ptmap for Mac builds

* [TravisCI] use the hererocks in python3 in mac builds

* use abstract field resolution instead of "implements Dynamic" for haxe.xml.Fast

this actually makes more lightweight due to less fields/allocations

* move haxe.web.Dispatch to hx3compat

* move magic types support code into their own module

* [java/cs] Fix IntMap implementation when setting 0 keys

Reviewed the whole Map implementations, added a few more comments
Closes #6457

* Disable toString test on C++

* [java] Add `no_map_cache` checks on WeakMap, and reimplement its iterators as classes

* Do not deploy binaries if we're running a PR

This only happened on PRs that were made by HF members, but still
we certainly don't want to submit them as nightlies

* [swf] rewrite safe-casts to if in non-retval mode

This is stupid. But it fixes #6482.

* [inliner] object declarations are _not_ constants

closes #6480

* disable failing test (see #6482)

* [eval] fix FileSystem path patching (closes #6481)

* [js] output file:line for traces (closes #6467)

* update submodule

* Add copy() to DynamicAccess (#5863)

* [xml] mention haxe.rtti.XmlParser (closes #6451)

* [display] allow dots in paths (closes #6435)

* [display] don't show private types in toplevel display (closes #6434)

* remove unused MSub module kind

* [matcher] print deprecation messages for enum (fields) (closes #6485)

* [parser] patch EField positions in reification (closes #6423)

* fix gadt pattern matching bug, closes #6413

* add interface tests

* [display] show constructor signatures on `new` (closes #6275)

* more local #if java for TestInterface

* make `in` a Binop and remove `EIn` (closes #6224) (#6471)

* make `in` a Binop and remove `EIn` (closes #6224)

* use make_binop

* fix enumIndex in tests

* change OpIn precedence to be lessthan OpAssign(Op)

* [js] fix tracing (closes #6492)

* minor

* Update JsonParser.hx (#6493)

haxe.format.JsonParser.parse("{\"\"\"A\":\"B\"}") -- any strings (empty string for sample), that are not separated by commas.

* add test for #6493

* [js] some doc on Symbol, add ofObject utility method and the standard `iterator` static field.

* supposedly fix #6491

* temp fix for #6467

* [js] setup proper keyword list based on selected ES version

* use rev_file_hash instead of rev_hash_s for eval position reporting so that it reports original file instead of unique_full_path

this also fixes positions multibyte utf8 files because original files are stored in lexer cache

* haxe now uses 1-based columns, so no need to +1 that in the eval debugger

* Split the Json parse test from the lambda arrow test until it is fixed.  Add shorcut for cpp on linux

* Add some additional cppia tests

* [make] don't add -native to ocamldep in bytecode mode

* casually walking into Mordor

* more comments

* more comment

* [lua] fix various coroutine-related signatures

* [lua] if dynamic is not table, return empty array for Reflect.fields

* [lua] Table helper methods

* [lua] do not generate function wrapping for explicit nulls (#6484)

* Make cppia use new ast by default.

* Update the docs for @:expose (#6515)

It works for Lua as well, and for both classes and fields.

* Fix duplicated platform in @:phpGlobal docs (#6518)

* [lua] fix Compiler.includeFile() with position = Inline

`haxe.macro.Compiler.includeFile("test.lua", "inline");` made `__js__` appear in the Lua output, leading to a rather funny error:

>attempt to call global '__js__' (a nil value)

* more comments for handle_efield

* remove weird loop from handle_efields. tests still pass, let's see what travis has to say.

* revert 36ea42d. this actually does something for untyped...

* more comments for handle_efield, also move the [] case to the bottom of the match for easier reading

* more comments

* add comment about the weird loop now when I get it :)

* more comments, also move the inner `loop` function closer to its usage

* add a couple more comments to handle_efield, remove unused argument from check_module (as it uses sname)

* add common decl_flag type for parsing common type flags instead of returning tuples of class+enum flag and then mapping it awkwardly

* Update @:selfCall docs to include Lua (#6523)

* Add a VSCode settings.json (#6524)

It's common practice to commit .vscode settings (eventually, it might be good to have a tasks.json and a launch.json too).

The first two are necessary to work with the vscode-ocaml extension. The files.exclude pattern prevents always ending up in _build files instead of the actual source files by accident (for instance via Ctrl+P, which used to show both).
Note that F1 -> Clear Editor History might be necessary for Ctrl+P not to show _build files anymore since it shows recently opened files regardless of the files.exclude setting.

[skip ci]

* Remove spaces around "in" in printBinop (#6527)

* [lua] properly escape reserved keywords when used as prototype declarations (#6528)

* [lua] convert all abort directives to error, in order to emit normalized line number/column info (#6519)

* [lua] change "failwith" to "error" for consistent error reporting

* [lua] speed up some array methods

* Revert "[lua] speed up some array methods"

This reverts commit f11ed3c.

* fix OpIn precedence: it's actually the lowest one. closes #6531

* Date.fromTime expects milliseconds (#6530)

* [matcher] support `@:forward` (closes #6532)

* Update MainLoop.hx (doc) (#6537)

Typo on call event's doc

* fixed generic getFloat to use FPHelper

* [eval] push block scope even for single-element blocks (see #6543)

* [eval] add constructor for haxe.io.Bytes (see #6543)

* [eval] print position for field index errors (#6543)

* [eval] support setting Bytes.length (see #6543)

* [matcher] don't leak `_` extractor variable (closes #6542)

* [js] create directory on setCustomJSGenerator (closes #6539)

* more binary support for js : use dataview for FPHelper, added dataview based BytesBuffer

* [lua] spacing nit

* [lua] do a better job marking haxe modules and classes as local

* [lua] speed up array methods (take 2)

* [TravisCI] ptmap 2.0.2 is compatible with ocaml 4.05

* [lua] fix unshift for empty array

* [lua] fix jit detection in tests

* [lua] de-inline a lengthy filesystem method

* [lua] fix unshift again

* [lua] fix array.shift for lua 5.3

* [lua] revert back to old array.unshift behavior

* [lua] use lua alias instead of luajit on hererocks installation

* [doc] fix Sys.getChar documentation (closes #6139)

* [lua] small std tweaks

* [js] initially grow BytesBuffer

* [inliner] deal with Dynamic returns (closes #6555)

* [matcher] don't let extractors access bindings (closes #6550)

* [matcher] fix extractor pattern offsets (closes #6548)

* [php7] do not throw on Reflect.getProperty(o, field) if field does not exist (fixes #6659, fixes 6554)

* unit test for #6559

* [cpp] Respect no_debug define

* [matcher] don't forget switch type when optimizing single-case matches (closes #6561)

* [tests] dodge C# issue (see #6561)

* unit test for #6502 (closes #6502)

* fixed some specific cases with empty buffer

* changed underlying TLazy implementation + make it a bit more abstract (allow to know if it's already processed or not)

* bugfix

* - error when accessing type currently being typed in macro
- don't assume not-method fields are not lazy typed
- flush core context (enter in PTypeExpr mode) when accessing a TLazy or calling field.expr()

close #683
close #5456

* I should have wait indeed

* fixed warning

* [TravisCI] install hererocks with --user

* force classes to be build after returning from a build call (same as we do when we load a type) close #6505

* partially revert fix for #6505

* better fix for #6505, do not flush if we are in module build (causes assert in typeload)

* don't consider hidden timers for timer alignment

* [eval] fix debug setVariable

* [inliner] don't mess up non-terminal try/catch (closes #6562)

* CHANGES.txt

* merge waneck-changes from nightly-travis branch
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

No branches or pull requests

3 participants